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: add Parse.Cloud.afterFind(Parse.File) #7927

Closed
wants to merge 27 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Apr 6, 2022

New Pull Request Checklist

Issue Description

Currently, there are no triggers for file gets.

Related issue: #6572, #7572, #7928

Approach

Adds preferred syntax Parse.Cloud.beforeSave(Parse.File)

The file triggers were made to have their own function names, which adds unnecessary complexity. This allows for Parse.Cloud.beforeSave(Parse.File), Parse.Cloud.afterSave(Parse.File), Parse.Cloud.beforeDelete(Parse.File), Parse.Cloud.afterDelete(Parse.File). In a future version, the current file triggers should be deleted.

Adds Parse.Cloud.beforeCreate(Parse.File)

This can only be used on Parse.File for the meantime, but in the future could be added to detect class creation. This triggers when a file is expanded, allowing mutation of the file url.

Adds Parse.Cloud.beforeFind

This allows a file trigger on the get request of a file. req.download can be set as well to allow file downloads.

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

mtrezza and others added 19 commits March 25, 2022 19:47
## [5.2.1-alpha.1](parse-community/parse-server@5.2.0...5.2.1-alpha.1) (2022-03-26)

### Bug Fixes

* return correct response when revert is used in beforeSave ([parse-community#7839](parse-community#7839)) ([f63fb2b](parse-community@f63fb2b))
## [5.2.1-alpha.2](parse-community/parse-server@5.2.1-alpha.1...5.2.1-alpha.2) (2022-03-26)

### Performance Improvements

* reduce database operations when using the constant parameter in Cloud Function validation ([parse-community#7892](parse-community#7892)) ([48bd512](parse-community@48bd512))
# [5.3.0-alpha.2](parse-community/parse-server@5.3.0-alpha.1...5.3.0-alpha.2) (2022-03-27)

### Bug Fixes

* security upgrade parse push adapter from 4.1.0 to 4.1.2 ([parse-community#7893](parse-community#7893)) ([ef56e98](parse-community@ef56e98))
# [5.3.0-alpha.4](parse-community/parse-server@5.3.0-alpha.3...5.3.0-alpha.4) (2022-04-04)

### Bug Fixes

* custom database options are not passed to MongoDB GridFS ([parse-community#7911](parse-community#7911)) ([a72b384](parse-community@a72b384))
@parse-github-assistant
Copy link

parse-github-assistant bot commented Apr 6, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@dblythy
Copy link
Member Author

dblythy commented Apr 6, 2022

This is still a bit of a proof of concept at this stage, more tests will need to be added prior to merge

@@ -8,6 +8,7 @@ The following is a list of deprecations, according to the [Deprecation Policy](h
| DEPPS2 | Config option `directAccess` defaults to `true` | [#6636](https://github.com/parse-community/parse-server/pull/6636) | 5.0.0 (2022) | 6.0.0 (2023) | deprecated | - |
| DEPPS3 | Config option `enforcePrivateUsers` defaults to `true` | [#7319](https://github.com/parse-community/parse-server/pull/7319) | 5.0.0 (2022) | 6.0.0 (2023) | deprecated | - |
| DEPPS4 | Remove convenience method for http request `Parse.Cloud.httpRequest` | [#7589](https://github.com/parse-community/parse-server/pull/7589) | 5.0.0 (2022) | 6.0.0 (2023) | deprecated | - |
| DEPPS5 | Remove file triggers in preference of normal `Parse.Cloud` syntax `Parse.Cloud.beforeDelete(Parse.File` | [#7927](https://github.com/parse-community/parse-server/pull/7927) |6.0.0 (2022) | 6.0.0 (2023) | deprecated | - |
Copy link
Member

@mtrezza mtrezza Apr 7, 2022

Choose a reason for hiding this comment

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

The deprecation would happen in 6.0 (2023), removal in 7.0 (2024).

Typo, missing closing bracket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as to my other PRs i'm not sure how to fill this table. How can an item's status be "Deprecated" if its planned Deprecation is Version 6?

@mtrezza
Copy link
Member

mtrezza commented Apr 7, 2022

This can only be used on Parse.File for the meantime, but in the future could be added to detect class creation.

I'd say the equivalent of Parse.File creation would be a Parse.Object creation, not a class creation. I can already imagine some use cases for that.

@dblythy
Copy link
Member Author

dblythy commented Apr 7, 2022

I'd say the equivalent of Parse.File creation would be a Parse.Object creation

Well I think a Parse.Object creation can be covered by wild class beforeSave triggers. In any case, it's out of scope of this PR. I personally think how the other beforeCreate triggers are implemented should be up for its own discussion.

@mtrezza
Copy link
Member

mtrezza commented Apr 7, 2022

The naming discussion is part of this PR because you are introducing a new trigger type. We want to find a name that is universal enough to be applicable to classes in the future. So let's think a few steps ahead...

In my understanding, beforeCreate is different from a beforeSave, just as creating/instantiating an object is different from saving it.

We may also soon have an internal File class as was discussed related to a few features. The beforeCreate trigger won't be triggered when the File class is created, nor when a File object is saved. A developer would expect the same behavior when the trigger is applied to other classes.

Maybe we should remove / rephrase the assumption that the will be related to class creation from the PR description to not confuse future readers about this?

@dblythy
Copy link
Member Author

dblythy commented Apr 7, 2022

Maybe the discussion for a beforeCreate trigger should happen elsewhere. I think it’s relevant to our discussions to have a trigger to mutate the file url. The name of that trigger might be better off with something else if you feel beforeCreate doesn’t represent its functions.

@mtrezza
Copy link
Member

mtrezza commented Apr 9, 2022

if you feel beforeCreate doesn’t represent its functions

The question is what would be "its function" when used with classes? I think beforeCreate is fine assuming that it can be used with classes for when an object is created, not for when a class is created. To know whether the new trigger name is well chosen in the sense that it can be repurposed for classes, we need to think ahead about what it could do when used with classes.

@mtrezza mtrezza force-pushed the alpha branch 2 times, most recently from 59215e6 to e6d7d8f Compare May 1, 2022 02:29
@dblythy
Copy link
Member Author

dblythy commented May 2, 2022

Closing - will reopen later

@dblythy dblythy closed this May 2, 2022
@dblythy
Copy link
Member Author

dblythy commented May 25, 2022

So you think beforeCreate should effectively be a beforeSave trigger but for !object.existed()? How would that affect the existing beforeSave trigger?

I also closed this and created #7966 to split it into smaller PRs. I will create a PR relating to this feature.

@mtrezza
Copy link
Member

mtrezza commented May 26, 2022

I would use beforeCreate to trigger when a Parse.Object instantiates.

  1. new Parse.Object() -> beforeCreate, for example to set default properties of an object
  2. Parse.Object.save() -> beforeSave, afterSave

You wrote:

This triggers when a file is expanded, allowing mutation of the file url.

What did you mean by "expanded"? What issue were you intending to solve with the beforeCreate(Parse.File) trigger?

@mtrezza mtrezza mentioned this pull request Oct 24, 2022
3 tasks
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.

6 participants