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

removed unnecessary check which causes nullable=true, handleNullableFields= true with value=null to fail in all cases except OneOf #554

Merged
merged 2 commits into from
Apr 18, 2022

Conversation

harishvashistha
Copy link
Contributor

@harishvashistha harishvashistha commented Apr 14, 2022

when nullable = true , handlenullableFields = true and value supplied = null , then getting this failure message "null found, string expected" for this example:

 "schema": {
      "properties": {
        "values": {
          "type": "string",
          "nullable": true,
          "default": null
        }
      }
    }

Data to be created:

"data": {
          "values": null
        }

Reason appears this unnecessary check for state.isComplexValidator() which is true only in case of oneOf . so it suppresses all handleNullableFields configurations causing the above error message.

This is related to my latest query on this issue #183

@harishvashistha harishvashistha changed the title removed unnecessary check removed unnecessary check which causes nullable=true, handleNullableFields= true with value=null to fail in all cases except OneOf Apr 14, 2022
@stevehu
Copy link
Contributor

stevehu commented Apr 14, 2022

@harishvashistha Thanks for your PR. I have run the test cases and they all passed from your branch. I cannot remember why we checked the complex validator in the first place but I guess there was a reason for that. I am wondering if we should handle the NULL for all validators or only a limited set of validators.

@harishvashistha
Copy link
Contributor Author

harishvashistha commented Apr 14, 2022

@stevehu Great, It would be great if we can get the suitable changes in official release of this library, we are eagerly waiting for this solution. Your help would be highly appreciated in our team.

@harishvashistha
Copy link
Contributor Author

harishvashistha commented Apr 15, 2022

@stevehu @prashanthjos should I merge this PR into another PR that is open #555 , which I created for another issue #553 ?

We need code changes of both the PRs, this as well as #555 , in your master library at the same time so that we can plan our product release bug free.

@stevehu
Copy link
Contributor

stevehu commented Apr 15, 2022

No worry. We are looking at both PRs together. Thanks.

@harishvashistha
Copy link
Contributor Author

@SiemelNaran @stevehu @prashanthjos can we get these changes in master release please. This PR is urgently required with PR #555 as we are planning a relase on Tuesday.
I hope you would understand the urgency for these changes.

@harishvashistha
Copy link
Contributor Author

@prashanthjos You are requested to please approve and merge this PR and if there are some review comments I can try to add and fix those quickly. This is the highest priority thing I am stuck with in my project.
I hope you would understand.

@stevehu stevehu merged commit cbac5b1 into networknt:master Apr 18, 2022
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