fix: determine type of combinedKeywords

fastify / fluent-json-schema

A fluent API to generate JSON schemas

MIT License

495 stars 58 forks source link

fix: determine type of combinedKeywords #237

Closed Uzlopak closed 6 months ago

Uzlopak commented 8 months ago

Resolves #233

Checklist

[ ] run npm run test and npm run benchmark
[ ] tests and/or benchmarks are included
[ ] documentation is changed or added
[ ] commit message and code follows the Developer's Certification of Origin and the Code of conduct

coveralls commented 8 months ago

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
Change from base Build 7654068412:	0.0%
Covered Lines:	420
Relevant Lines:	420

💛 - Coveralls

mcollina commented 8 months ago

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

Uzlopak commented 8 months ago

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 commented 8 months ago

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

giovanni-bertoncelli commented 8 months ago

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

Uzlopak commented 6 months ago

Closing due to inactivity.

giovanni-bertoncelli commented 6 months ago

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 commented 6 months ago

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.