Skip to content

Don't error when attempting to sort on an object field #4806

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 11 commits into from
Jun 7, 2018

Conversation

acinader
Copy link
Contributor

@acinader acinader commented Jun 2, 2018

fixes: #4805

see

fieldName = fieldName.split('.')[0];

There is a validation check for the name of a field to make sure that it is valid. This check is performed in both the Schema Controller and the Database Controller. In the database controller, the check is applied when an update or a find operation is requested on the database.

Since updates can occur to and find's can be performed on members of a document, it is important that the check only be performed on the name of the field, and not the member.

In the case of the update, there is an existing split that separates the field from the member.

The split is missing from the find when trying to sort on a field which is erroneously causing an error.

This pull request introduces a test to sort on a member of a document and a fix to allow the test to pass.

@acinader acinader requested a review from flovilmart June 2, 2018 00:01
@acinader
Copy link
Contributor Author

acinader commented Jun 2, 2018

well postgress doesn't like it.

@acinader acinader requested a review from vitaly-t June 4, 2018 16:17
@acinader
Copy link
Contributor Author

acinader commented Jun 4, 2018

@vitaly-t any feedback on the postgress failure?

@codecov
Copy link

codecov bot commented Jun 4, 2018

Codecov Report

Merging #4806 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4806      +/-   ##
==========================================
- Coverage   92.69%   92.69%   -0.01%     
==========================================
  Files         119      119              
  Lines        8684     8688       +4     
==========================================
+ Hits         8050     8053       +3     
- Misses        634      635       +1
Impacted Files Coverage Δ
src/Controllers/DatabaseController.js 94.67% <100%> (+0.03%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.16% <100%> (ø) ⬆️
src/RestWrite.js 93.61% <0%> (-0.19%) ⬇️

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 cf3a872...549c22a. Read the comment docs.

@vitaly-t
Copy link
Contributor

vitaly-t commented Jun 4, 2018

No, I never touched the generic database code here, only the driver-related stuff.

@dplewis
Copy link
Member

dplewis commented Jun 7, 2018

@acinader The fix for PG should be a simple fix. Transform sortField.value into sortField ->> 'value' or something like that. I'm sure you can order by json object value.

@flovilmart
Copy link
Contributor

So actually in the validation, we should allow the dot notation / transformation of this sorting no?

@dplewis
Copy link
Member

dplewis commented Jun 7, 2018

@flovilmart I know this issue #2113 has needed addressing. I can add support for PG order by nested since mongo already does.

@flovilmart
Copy link
Contributor

Go ahead, perhaps à PR on @acinader’s or use a branch on this repo so you guys can contribute together

@dplewis
Copy link
Member

dplewis commented Jun 7, 2018

@acinader @flovilmart I added some additional test. How does this look?

@acinader
Copy link
Contributor Author

acinader commented Jun 7, 2018

@dplewis awesome! thanks.

@flovilmart comment added. I factored out the operation into a function and commented. Got rid of param reassignment too, bonus.

@flovilmart
Copy link
Contributor

Yeah that’s better. I would probably have called it ‘rootFieldName’ but that’s ok as is :)

@flovilmart flovilmart merged commit e064716 into parse-community:master Jun 7, 2018
@acinader
Copy link
Contributor Author

acinader commented Jun 7, 2018

yeah, that is a lot better name. see #4817. thanks.

flovilmart pushed a commit that referenced this pull request Aug 12, 2018
* add failing test to demonstrate that you can't sort on a
field in an object.

* Only validate the base of the field name.

* fix test name

* Only test sort for mongo.

* pg order by nested object

* level 2 test

* Factor out operation to get a field's base name.  Add comment.

* tweak comment wording so it wont make my grammar teacher angry.
flovilmart pushed a commit that referenced this pull request Aug 12, 2018
* add failing test to demonstrate that you can't sort on a
field in an object.

* Only validate the base of the field name.

* fix test name

* Only test sort for mongo.

* pg order by nested object

* level 2 test

* Factor out operation to get a field's base name.  Add comment.

* tweak comment wording so it wont make my grammar teacher angry.
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
…ty#4806)

* add failing test to demonstrate that you can't sort on a
field in an object.

* Only validate the base of the field name.

* fix test name

* Only test sort for mongo.

* pg order by nested object

* level 2 test

* Factor out operation to get a field's base name.  Add comment.

* tweak comment wording so it wont make my grammar teacher angry.
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.

Allow a query to sort based on an object value field
4 participants