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

Implement SerDes.jsonType option for non-object internal types. #632

Merged
merged 2 commits into from
Sep 12, 2021

Conversation

robertjustjones
Copy link
Contributor

The current SerDes assumes an object type on deserialization because the schema preprocessor hardcodes that jsonType. While arrays are objects in JS, they are distinct in JsonType so arrays will fail the object type check.

This PR adds an option to specify an alternative like "array" jsonType and passes it through in the preprocessor code. Included are an added test and doc update.

Thanks for the work on this functionality @pilerou and @cdimascio.

@robertjustjones
Copy link
Contributor Author

I've since implemented a check for request data being a string before deserialization, removing skip that had been implemented if the incoming data was already of type object. Without this check, the above jsonType="array" would allow an array to be submitted instead of enforcing the string requirement in the API Spec.

You may want another option (strict?) for this check so I have PR'd it, but you can see the code and additional test here.

robertjustjones@60d34af

Copy link
Owner

@cdimascio cdimascio left a comment

Choose a reason for hiding this comment

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

@robertjustjones this is great. thank you

@cdimascio cdimascio merged commit 01f5b5c into cdimascio:master Sep 12, 2021
@cdimascio
Copy link
Owner

@all-contributors add @robertjustjones for code and test

@allcontributors
Copy link
Contributor

@cdimascio

I've put up a pull request to add @robertjustjones! 🎉

@robertjustjones
Copy link
Contributor Author

Thanks @cdimascio!

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.

2 participants