Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: determine type of combinedKeywords #237

Closed
wants to merge 1 commit into from
Closed

fix: determine type of combinedKeywords #237

wants to merge 1 commit into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 26, 2024

Resolves #233

Checklist

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7673994061

  • 0 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7654068412: 0.0%
Covered Lines: 420
Relevant Lines: 420

💛 - Coveralls

@@ -224,7 +224,9 @@ describe('S', () => {
})

describe('compose keywords', () => {
const ajv = new Ajv()
const ajv = new Ajv({
allowUnionTypes: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ajv will warn and spam the log output, because here type will be integer and string.

https://ajv.js.org/strict-mode.html#union-types

properties: { foo: { type: 'string', anyOf: [{ type: 'string' }] } },
type: 'object'
})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a similar test for oneOf?

@mcollina
Copy link
Member

Looking at the change, this seems counterintuitive. Why specifying the same thing twice?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 27, 2024

I just implemented it to fix the reported issue. If ajv strict is warning, because type is missing, then i guess it needs to be added, even if it is twice.

Should I continue on this PR or should we wait for more feedback?

@mcollina
Copy link
Member

Looking at it, it doesn't seems something we should fix. I've actually never used type with anyOf.

@giovanni-bertoncelli
Copy link

giovanni-bertoncelli commented Jan 29, 2024

I've not found any JSONSchema specs that actually requires "type" with oneOf/anyOf.. maybe we should consider raising #233 on AJV side?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 10, 2024

Closing due to inactivity.

@Uzlopak Uzlopak closed this Apr 10, 2024
@Uzlopak Uzlopak deleted the fix-233 branch April 10, 2024 18:48
@giovanni-bertoncelli
Copy link

I've not found any JSONSchema specs that actually requires "type" with oneOf/anyOf.. maybe we should consider raising #233 on AJV side?

I've been waiting for some reply on this.. I have still a lot of spam in my logs for this issue..

@climba03003
Copy link
Member

I've been waiting for some reply on this.. I have still a lot of spam in my logs for this issue..

https://json-schema.org/understanding-json-schema/reference/combining#factoringschemas

type is not a requirement when using oneOf, anyOf or allOf.
All the additional properties when using oneOf, anyOf or allOf should be the common part. I don't think it is good to have union type there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object type ignored when nested properties use oneOf and raw
5 participants