Skip to content

Bug: null value not persisted for properties of type JSON, Any, or Object #1896

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

Closed
wants to merge 2 commits into from
Closed

Conversation

ewrayjohnson
Copy link

@ewrayjohnson ewrayjohnson commented Jul 13, 2021

Fixes #1895
Inadvertently includes PR from @mvertopoulos [Fix for allowing includes where related id (targetId) is zero]

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

@slnode
Copy link

slnode commented Jul 13, 2021

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@ewrayjohnson ewrayjohnson changed the title null value not persisted for properties of type JSON, Any, or Object Bug: null value not persisted for properties of type JSON, Any, or Object Jul 13, 2021
@dhmlau
Copy link
Member

dhmlau commented Jul 14, 2021

@ewrayjohnson, thanks for the PR. Your changes look good to me. Could you please add some tests to verify your changes?
Also, please take care of the following as well:

  • sign the DCO
  • there's a lint error for 183:64 error Trailing spaces not allowed no-trailing-spaces
  • commit linter error for 49c8d679c5 - Fix for allowing includes where related id (targetI: First line should be 50 characters or less (saw 61)
    Thanks!

@ewrayjohnson
Copy link
Author

ewrayjohnson commented Jul 14, 2021

@dhmlau: I will gladly complete the items you requested, however, I made some mistakes and still don't know the best way to fix it.

  1. I need to apply my fix at version 3.x since my (legacy) app is still using LoopBack 3.x. Did I do that correctly?
  2. I inadvertently mixed in a PR from mvertopoulos (49c8d67 - Fix for allowing includes where related id). When I went to add a PR his was just there and I could not create mine without his. I would love to create a new one only with my change.
  3. I cannot clone my PR locally to write the tests.
  4. I don't know how to properly fork at 3.x. Somehow I create a repo with the correct branch and my fix but it is disconnected from the official repo. Here is my repo https://github.com/ewrayjohnson/loopback-datasource-juggler/tree/pr/1892
  5. Since my repo is disconnected, I cannot do a diff.
    HELP

@dhmlau
Copy link
Member

dhmlau commented Jul 14, 2021

Thanks @ewrayjohnson.

I need to apply my fix at version 3.x since my (legacy) app is still using LoopBack 3.x. Did I do that correctly?

Yes. 3.x branch is for LoopBack 3.0. and master is for LoopBack 4. When this PR has landed, do you mind submit the fix to master as well?

I don't know how to properly fork at 3.x. Somehow I create a repo with the correct branch and my fix but it is disconnected from the official repo.

If you run git remote -v, I assume it's not pointing to this repo, that's why you concluded that your repo is disconnected? One thing you could try is to add the remote again. i.e.

git remote add upstream https://github.com/loopbackio/loopback-datasource-juggler.git <-- add the remote
git remote -v  <--- verify what you've set up

Hope it helps!

@stale
Copy link

stale bot commented Sep 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 12, 2021
@stale
Copy link

stale bot commented Sep 26, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants