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

feat: Support dot notation on array fields #9115

Open
wants to merge 7 commits into
base: alpha
Choose a base branch
from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented May 2, 2024

Pull Request

Issue

Currently all dot notations are treated as operations on Object type field. This results in a error: Cannot read property '0' of undefined when used on Array type fields.

Closes: #6687

Approach

  • Check types for Array vs Fields based on dot notation format.
  • If dot notation is used prevent array to object conversion

https://www.mongodb.com/docs/manual/core/document/#dot-notation

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

Copy link

Thanks for opening this pull request!

@dplewis dplewis requested a review from a team May 2, 2024 16:16
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.77%. Comparing base (2ecc5a5) to head (7e57ccb).

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9115      +/-   ##
==========================================
- Coverage   93.78%   93.77%   -0.02%     
==========================================
  Files         186      186              
  Lines       14729    14737       +8     
==========================================
+ Hits        13814    13820       +6     
- Misses        915      917       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza mtrezza changed the title feat: Support dot notation on Array fields feat: Support dot notation on array fields May 3, 2024
mtrezza
mtrezza previously approved these changes May 3, 2024
@dplewis
Copy link
Member Author

dplewis commented May 5, 2024

@mtrezza This is an extremely complicated feature. I tried running this with the SDK internal fix against the server test suite and got Array dot notation working but it somehow broke the server. I’ll let you know when this is ready for review.

@mtrezza mtrezza marked this pull request as draft May 6, 2024 00:32
@dplewis dplewis marked this pull request as ready for review July 7, 2024 01:27
@dplewis
Copy link
Member Author

dplewis commented Jul 7, 2024

@mtrezza This is ready for review.

@dplewis dplewis requested review from mtrezza and a team July 7, 2024 01:30
// JSON Arrays are treated as Nested Objects
const [x, y] = fieldName.split('.');
fieldName = x;
if (!isNaN(y) && !['sentPerUTCOffset', 'failedPerUTCOffset'].includes(fieldName)) {
Copy link
Member

@mtrezza mtrezza Jul 7, 2024

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the stricter array index check.

The hard coded field names come from the _PushStatus schema. They have array indexes but are saved as objects instead of arrays.

Copy link
Member

@mtrezza mtrezza Jul 7, 2024

Choose a reason for hiding this comment

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

They have array indexes but are saved as objects instead of arrays.

What does that mean? Is this the issue discussed in parse-community/Parse-SDK-JS#2198 (comment) where we don't know whether an object key 0 is an array index or a normal object key? I have rebased this PR to include the tests added in #9179. Let's see how that goes.

Copy link
Member

Choose a reason for hiding this comment

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

The tests pass, that's a good sign. What would we have to change for the push status so we don't have to treat them differently here?

Copy link
Member Author

@dplewis dplewis Jul 7, 2024

Choose a reason for hiding this comment

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

Pretty much nothing we can do here. These are built into Parse instead of created by the user. This wouldn't break anything as the fields are type Object in the schema and are still type Object after this PR.

If you want to see how it works check out this test

https://github.com/parse-community/parse-server/blob/alpha/spec/PushWorker.spec.js#L324

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.

How to increment array nested object field?
2 participants