Skip to content

fix: null value not persisted for properties of type JSON, Any, or Ob… #2080

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

Merged
merged 1 commit into from
Jun 20, 2023
Merged

fix: null value not persisted for properties of type JSON, Any, or Ob… #2080

merged 1 commit into from
Jun 20, 2023

Conversation

siimsams
Copy link
Contributor

Fix the following issue on latest master.

fixes #1895

Checklist

  • Sign off your commits with DCO (Developer Certificate of Origin)
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@siimsams siimsams requested a review from dhmlau as a code owner June 19, 2023 15:28
@siimsams
Copy link
Contributor Author

I have added tests aswell. Please let me know if there is anything missing here. I'd really like to get this merged as we have many projects with this issue.

@ewrayjohnson
Copy link

@siimsams: Thanks. Although I proposed the fix, I had not found the time to create the tests.

@siimsams
Copy link
Contributor Author

siimsams commented Jun 20, 2023

@siimsams: Thanks. Although I proposed the fix, I had not found the time to create the tests.

Thank you for proposing the fix. I added the tests aswell. Hope this gets approved and merged. We want to use latest versions but this issue blocks us from doing that.

"message": "invalid input syntax for type integer: "null"",
"stack":
error: invalid input syntax for type integer: "null"

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

@siimsams, thanks for your contribution. Please see my comments. I'll take a closer look at the change later today. Thanks.

@siimsams siimsams requested a review from dhmlau June 20, 2023 15:04
@siimsams
Copy link
Contributor Author

@siimsams, thanks for your contribution. Please see my comments. I'll take a closer look at the change later today. Thanks.

Thank you for such fast response! I have fixed the issues.

@coveralls
Copy link

coveralls commented Jun 20, 2023

Pull Request Test Coverage Report for Build 5324909921

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 84.814%

Totals Coverage Status
Change from base Build 5308588606: 0.1%
Covered Lines: 7267
Relevant Lines: 8264

💛 - Coveralls

@siimsams
Copy link
Contributor Author

siimsams commented Jun 20, 2023

Fixed the commit message. It would be nice to have these required prefixes detailed in the pr/commit guidelines. As i could not have known about the requirement beforehand.

@dhmlau
Copy link
Member

dhmlau commented Jun 20, 2023

@siimsams, there's still error from the commit linter

   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test] [type-enum]

I suggest to squash the commit and have the commit message something like what you have in the title (make sure it's within 50 characters).

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

Your changes LGTM. Let's get the commit linting error fixed and we can go ahead to merge. Thanks.

@siimsams
Copy link
Contributor Author

siimsams commented Jun 20, 2023

Squashed everything. Should be good to go now.

Your changes LGTM. Let's get the commit linting error fixed and we can go ahead to merge. Thanks.

Edit:
Looks like everything passed. LGTM!

@siimsams siimsams requested a review from dhmlau June 20, 2023 18:45
@dhmlau dhmlau merged commit ae3cf25 into loopbackio:master Jun 20, 2023
@dhmlau
Copy link
Member

dhmlau commented Jun 20, 2023

@siimsams, thanks for your contribution. Your PR has landed! 🎉

@siimsams
Copy link
Contributor Author

Thank you so much!

@siimsams siimsams deleted the fix/1895-v4.28 branch June 21, 2023 04:05
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.

null value not persisted for properties of type JSON, Any, or Object
4 participants