Read preference option #3865
Read preference option #3865
Conversation
src/rest.js
Outdated
| enforceRoleSecurity('get', className, auth); | ||
| const query = new RestQuery(config, auth, className, { objectId }, restOptions, clientSDK); | ||
| return query.execute(); | ||
| return triggers.maybeRunQueryTrigger(triggers.Types.beforeFind, className, restWhere, restOptions, config, auth).then((result) => { |
There was a problem hiding this comment.
we should probably make the difference between a get and a find here (as they yield different results)
There was a problem hiding this comment.
I think it would be useful but it will be annoying. We don't have a safe way to be 100% sure that the frontend is trying to do a get and not a find. The js SDK uses the find endpoint for getting (https://github.com/parse-community/Parse-SDK-JS/blob/master/src/ParseQuery.js#L284). That's why this test (https://github.com/parse-community/parse-server/blob/master/spec/CloudCode.spec.js#L117) already works in the master without this change but this new one (https://github.com/parse-community/parse-server/pull/3865/files#diff-57110aba585acf52c5d9209a61443a8eR1309) doesnt. Therefore I think the best way for someone to check if it is a get or a find is by checking if the where query contains only objectId among the keys.
There was a problem hiding this comment.
This si a Parse.Query so that makes sense it's a find, I was thinking more of the fetches, or all the calls that start with a get (where we can know it came from the 'get' endpoints, and not on the 'find' endpoints)
Also, we need to be sure as we have CLP that are based on GET vs FIND :)
There was a problem hiding this comment.
Ok. Im convinced :)
Just done
spec/ReadPreferenceOption.spec.js
Outdated
| obj1.set('boolKey', true); | ||
|
|
||
| Parse.Object.saveAll([obj0, obj1]).then(() => { | ||
| const hook = { |
There was a problem hiding this comment.
@davimacedo why don't you just put a spy on the database.serverConfig.cursor and validate the spy later on (you can get all arguments etc...
There was a problem hiding this comment.
I copied the same idea of mongo driver :) (https://github.com/mongodb/node-mongodb-native/blob/2.2/test/functional/readpreference_tests.js)
But I agree it is better with spy. Just improved.
spec/ReadPreferenceOption.spec.js
Outdated
| const ReadPreference = require('mongodb').ReadPreference; | ||
| const rp = require('request-promise'); | ||
|
|
||
| describe('Read preference option', () => { |
There was a problem hiding this comment.
should be a describe_only_db('mongo'), not sure it applies to PG yet. That will alleviate confusion.
spec/ReadPreferenceOption.spec.js
Outdated
| }; | ||
| spyOn(hook, 'method').and.callThrough(); | ||
|
|
||
| const cursor = databaseAdapter.database.serverConfig.cursor; |
There was a problem hiding this comment.
the databaseAdapter should probably be acquired by new Config('test').databaseController.adapter
davimacedo
force-pushed
the
readPreferenceOption
branch
from
03c4da4 to
2a30943
Compare
Codecov Report
@@ Coverage Diff @@
## master #3865 +/- ##
==========================================
+ Coverage 90.55% 90.56% +0.01%
==========================================
Files 115 115
Lines 7741 7783 +42
==========================================
+ Hits 7010 7049 +39
- Misses 731 734 +3
Continue to review full report at Codecov.
|
davimacedo
force-pushed
the
readPreferenceOption
branch
from
2a30943 to
025a451
Compare
|
This is looking great! Just needs a small rebase to integrate the lastest changes from master! |
|
@davimacedo bump? |
davimacedo
force-pushed
the
readPreferenceOption
branch
from
025a451 to
b6ca0ff
Compare
|
Sorry about the delay. Just done. |
|
@davimacedo looks like it needs another rebase unfortunately :/ |
davimacedo
force-pushed
the
readPreferenceOption
branch
from
b6ca0ff to
87ba42a
Compare
davimacedo
force-pushed
the
readPreferenceOption
branch
from
87ba42a to
210658f
Compare
|
Done! |
No description provided.