Skip to content

fix: determine type of combinedKeywords - #237

Closed
Uzlopak wants to merge 1 commit into
masterfrom
fix-233
Closed

Uzlopak wants to merge 1 commit into
masterfrom
fix-233

Conversation

@Uzlopak

@Uzlopak Uzlopak commented Jan 26, 2024

Copy link
Copy Markdown
Contributor

Resolves #233

Checklist

@coveralls

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/FluentSchema.test.js
properties: { foo: { type: 'string', anyOf: [{ type: 'string' }] } },
type: 'object'
})
})

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@Uzlopak

Uzlopak commented Jan 27, 2024 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@ghost

ghost commented Jan 29, 2024 •

Copy link
Copy Markdown

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

@Uzlopak

Uzlopak commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

Closing due to inactivity.

@Uzlopak Uzlopak closed this Apr 10, 2024
@Uzlopak
Uzlopak deleted the fix-233 branch April 10, 2024 18:48
@ghost

ghost commented Apr 11, 2024

Copy link
Copy Markdown

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
Copy Markdown
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.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

4 participants