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

Integers as Discriminators #2419

Closed
b-wils opened this issue Apr 18, 2024 · 7 comments
Closed

Integers as Discriminators #2419

b-wils opened this issue Apr 18, 2024 · 7 comments

Comments

@b-wils
Copy link

b-wils commented Apr 18, 2024

What version of Ajv you are you using?

8.12.0

What problem do you want to solve?

I have an existing schema where I'm trying to use it's integer properties as discriminators.

It seems that ajv discriminators only allow integers. I'm not sure if this is an implementation limitation or something defined by the OpenAPI spec.

What do you think is the correct solution to problem?

Update the discriminator to allow integers. Ideally the integer can just be converted to a string for the map lookup.

Will you be able to implement it?

Possibly

@jasoniangreen
Copy link
Collaborator

Hi there, could you put together a runkit exhibiting the issue? Try and keep the example as simple as possible.

@b-wils
Copy link
Author

b-wils commented Apr 22, 2024

https://runkit.com/b-wils/ajv-number-discriminator

@jasoniangreen
Copy link
Collaborator

Judging by the example here https://ajv.js.org/json-schema.html#discriminator your schema is not quite right.

Looks like you are missing propertyName in discriminator: {propertyName: "foo"}.

@b-wils
Copy link
Author

b-wils commented Apr 27, 2024

Ah yes, my apologies. https://runkit.com/b-wils/ajv-number-discriminator is updated with the correct discriminator declaration and now should demonstrate the issue with numbers as discriminators.

@jasoniangreen
Copy link
Collaborator

Thanks for that. So after reviewing things I can see that it is indeed a deliberate restriction and there was actually a PR a while back to add support for other scalar data types.

I am just in the process of preparing a release (the first in a long time) but I can look into this afterwards and follow up with @epoberezkin.

@jasoniangreen
Copy link
Collaborator

Hi @b-wils, I have spoken to EP and this is not a change we're open to exploring at this time. I don't have all the context but there are reasons it should be a string and my interpretation of the spec is that it should be a string too. It should be pretty trivial to work around too.

@b-wils
Copy link
Author

b-wils commented May 6, 2024

Thanks for looking into it, if this is isn't part of the spec then that makes sense to not extend it in ajv.

By workaround, you mean change the fields from numbers to strings? Unfortunately our data is already in use and won't be feasible to change existing users. Replacing oneOf with if/then is a (slightly awkward) workaround to reduce error noise, or we may just live with the noise.

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

No branches or pull requests

2 participants