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

Make possible to alter response using the after save trigger #5814

Merged
merged 4 commits into from
Jul 25, 2019
Merged

Make possible to alter response using the after save trigger #5814

merged 4 commits into from
Jul 25, 2019

Conversation

brunoMaurice
Copy link
Contributor

The trigger response is only handle for beforeSave and afterFind event let handle it also for afterSave.

In my use case, I do some data manipulation/injection using the beforeSave trigger and I don't want to show them. Since it's not possible to define an hidden field (like the hard coded field password), It would be great to be able to alter the object using after-save like we can do it with after-find.

Let me know if you have some suggestion or question about the change :)

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #5814 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5814      +/-   ##
==========================================
- Coverage   93.67%   93.66%   -0.02%     
==========================================
  Files         146      146              
  Lines       10236    10242       +6     
==========================================
+ Hits         9589     9593       +4     
- Misses        647      649       +2
Impacted Files Coverage Δ
src/triggers.js 94.64% <100%> (+0.09%) ⬆️
src/RestWrite.js 93.36% <100%> (-0.32%) ⬇️

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 cdc6eee...f782a8f. Read the comment docs.

@acinader

This comment has been minimized.

@acinader
Copy link
Contributor

  1. this seems reasonable as a feature (I haven't studied the code yet and don't have time at the moment to fully consider it, but will if @davimacedo or @dplewis don't beat me to it.)

  2. In your pr comment, you say: 'it's not possible to define a hidden field', thanks to a nice, recent addition that isn't documented, you can now with protected fields, see: https://github.com/parse-community/parse-server/blob/master/spec/ProtectedFields.spec.js

by using protected fields, you can mark a field as master only, or give it permission for any other role or pointer.

@brunoMaurice
Copy link
Contributor Author

brunoMaurice commented Jul 15, 2019

Hi @acinader, thanks for the quick answer.

I will check the ProtectedFields.spec.js and who I set it up.
I started to use it but I didn't see change. Probably miss configured :/

@davimacedo
Copy link
Member

davimacedo commented Jul 15, 2019

@brunoMaurice Thanks for the PR! But I actually didn't understand very well the use case. The REST API only returns createdAt and objectId fields when creating a new object, and updatedAt field when updating an existing object. So actually all fields are hidden by design. I am not sure if we should change this behavior since the SDKs do not know what to do with you add something different than that to the response.

@brunoMaurice
Copy link
Contributor Author

If we follow the documentation it should be like that but if you have a beforeSave like that :

Parse.Cloud.beforeSave(schemaClass.className, function(req) {
      if (!req.user) {
        return;
      }

      req.object.set('owner', req.user);

      const roleACL = new Parse.ACL();
      roleACL.setWriteAccess(req.user, true);
      roleACL.setRoleReadAccess('Developer', true);

      req.object.set('ACL', roleACL);
});

The response of the request will have owner and ACL field instead of only createdAt and ob jectId or updatedAt. It's maybe not expected but I had to deal with it :/

@davimacedo
Copy link
Member

In my opinion, that's a weird behavior of Parse Server but you are right. Just checked here and it seems that it was designed to send back the changed fields in the before save trigger so the client can be aware of what happened there.

Considering this, I think your PR makes totally sense and I will review it right now.


it('test afterSave response object is return', done => {
Parse.Cloud.afterSave('TestObject2', function(req) {
const jsonObject = req.object.toJSON();
Copy link
Member

Choose a reason for hiding this comment

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

I think it is not so straightforward for others users to use this capability. Can you test just using the code below?

req.object.unset('todelete');
req.object.set('toadd', true);
return req.object;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually an other issue I had. If you do that you will have :

{
  "tokeep": true,
  "createdAt": "2019-07-18T09:12:58.496Z",
  "todelete": {
    "__op": "Delete"
  },
  "updatedAt": "2019-07-18T09:12:58.496Z",
  "toadd": true,
  "objectId": "zC2AmMvGxM"
}

the __op is not really great response :/

Copy link
Member

Choose a reason for hiding this comment

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

Can you try like this?

req.object.unset('todelete');
req.object.set('toadd', true);
return req.object._toFullJSON();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With that I have :

{ todelete: { __op: 'Delete' },
     tokeep: true,
     createdAt: '2019-07-25T13:49:32.552Z',
     updatedAt: '2019-07-25T13:49:32.552Z',
     toadd: true,
     objectId: '4lgUT2xWo5',
     __type: 'Object',
     className: 'TestObject2' }

with toJson :

{ todelete: { __op: 'Delete' },
     tokeep: true,
     createdAt: '2019-07-25T13:49:32.552Z',
     updatedAt: '2019-07-25T13:49:32.552Z',
     toadd: true,
     objectId: '4lgUT2xWo5' }

src/RestWrite.js Outdated Show resolved Hide resolved
src/triggers.js Show resolved Hide resolved
@brunoMaurice
Copy link
Contributor Author

brunoMaurice commented Jul 18, 2019

The test is running fine in local don't get what's wrong with the CI :s

@davimacedo davimacedo merged commit 50f1e8e into parse-community:master Jul 25, 2019
@brunoMaurice brunoMaurice deleted the save-after-handle-response branch July 29, 2019 12:14
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
…ommunity#5814)

* make possible to alter response using the after save trigger like for after find

* code clearing to follow same object checking

* remove console log debug

* fix test unit
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.

3 participants