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.

Sign up for GitHub

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

Jump to bottom

Fixed a bug affecting updates to nested pointers #7392

Merged
merged 2 commits into from
May 25, 2021
Merged

Fixed a bug affecting updates to nested pointers #7392

merged 2 commits into from
May 25, 2021

Conversation

mstniy
Copy link
Contributor

@mstniy mstniy commented May 19, 2021 •
edited
Loading

Also created unit tests

New Pull Request Checklist

Issue Description

Updates to nested pointers don't work

Related issue: #7391

Approach

MongoTransform avoids prefixing nested keys with _p_, even if they contain the "__type: Pointer" field.
transformQueryKeyValue invokes transformInteriorAtom if the key is nested (contains a dot)

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • ...

@codecov
Copy link

codecov bot commented May 19, 2021 •
edited
Loading

Codecov Report

Merging #7392 (a18687a) into master (38c01c6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a18687a differs from pull request most recent head 3475107. Consider uploading reports for the commit 3475107 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7392   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files         181      181           
  Lines       13209    13210    +1     
=======================================
+ Hits        12405    12406    +1     
  Misses        804      804
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js 88.82% <100.00%> (+0.01%) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 79.50% <0.00%> (-0.82%) ⬇️
src/RestWrite.js 94.08% <0.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38c01c6...3475107. Read the comment docs.

@davimacedo
Copy link
Member

Thanks for the PR. It looks that the tests are not passing for Postgres. Could you please take a look?

@mstniy
Copy link
Contributor Author

mstniy commented May 21, 2021

The new test fails on PostgreSQL, likely because the issue remains. Unfortunately, our use case does not involve PostgreSQL so we have limited motivation to fix the PostgreSQL adapter. Perhaps we can mark the new test as pending on PostgreSQL?

davimacedo
davimacedo requested changes May 24, 2021
View reviewed changes
src/Adapters/Storage/Mongo/MongoTransform.js Outdated Show resolved Hide resolved
spec/ParseAPI.spec.js Outdated Show resolved Hide resolved
@mstniy
Marked the regression test for parse-community#7391 as pending for po…
3475107 
…stgre

  The issue is not fixed yet
Use cont instead of var
davimacedo
davimacedo approved these changes May 24, 2021
View reviewed changes
Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

Since it is a bug, I am fine in merging this. It is better to have it fixed only for MongoDB than to not have it fixed for neither of them. @dplewis @mtrezza thoughts?

@mtrezza
Copy link
Member

mtrezza commented May 24, 2021 •
edited
Loading

Agree; MongoDB is clearly the more important adapter, updating it should not be held back by another adapter.

@davimacedo davimacedo merged commit 5e7c9d2 into parse-community:master May 25, 2021
@mstniy mstniy deleted the nested_pointers_7391 branch May 26, 2021 07:53
@mstniy mstniy mentioned this pull request May 26, 2021
Open
6 tasks
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davimacedo davimacedo davimacedo approved these changes

Assignees
No one assigned
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mstniy @davimacedo @mtrezza @parseplatformorg