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

fix: Parse.Cloud.startJob and Parse.Push.send not returning status ID when setting Parse Server option directAccess: true #8766

Merged
merged 15 commits into from
Apr 14, 2024

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Sep 27, 2023

Pull Request

Issue

Calling Parse.Cloud.startJob or Parse.Push.send from a Cloud Code function while directAccess is false doesn't return the status Id.

Closes: #8770
Related: parse-community/Parse-SDK-JS#2033

Approach

Support status Id headers in the direct access REST Controller

Tasks

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

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 27, 2023

Thanks for opening this pull request!

@mtrezza mtrezza removed their assignment Sep 27, 2023
@mtrezza mtrezza changed the title fix: Return Parse.Job and Parse.Push statusId when directAccess is false fix: Parse.Job and Parse.Push not returning status ID when Parse Server option directAccess: true Sep 27, 2023
@mtrezza mtrezza changed the title fix: Parse.Job and Parse.Push not returning status ID when Parse Server option directAccess: true fix: Parse.Cloud.startJob and Parse.Push not returning status ID when Parse Server option directAccess: true Sep 27, 2023
@mtrezza mtrezza changed the title fix: Parse.Cloud.startJob and Parse.Push not returning status ID when Parse Server option directAccess: true fix: Parse.Cloud.startJob and Parse.Push.send not returning status ID when Parse Server option directAccess: true Sep 27, 2023
@mtrezza mtrezza changed the title fix: Parse.Cloud.startJob and Parse.Push.send not returning status ID when Parse Server option directAccess: true fix: Parse.Cloud.startJob and Parse.Push.send not returning status ID when setting Parse Server option directAccess: true Sep 27, 2023
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

This PR provides a quick fix, but I'm not sure it's a sustainable fix as it duplicates codes across 2 repositories which will be difficult to maintain.

The underlying issue seems to be that the REST controller makes the decision to convert response headers as needed for methods (runJob, sendPush). This has the pattern of a circular dependency where the method depends on the REST controller returning the headers and the REST controller depends on the methods to decide which headers to return.

The REST controller should be caller-agnostic. If there are any response headers it should provide them to the caller, so the caller decides which headers to process further. Just like it does with the response body.

Just imagine that in the future runJob should return the status ID and the value from another header. We would need to start building the response object of runJob in the REST controller which doesn't make sense.

Could the REST Controllers simply pass an additional headers object back to the calling function? This way the runJob can pick up the header and compose the response. This would avoid code duplication, work regardless of REST controller and it provides us more flexibility in the future.

@dplewis
Copy link
Member Author

dplewis commented Sep 28, 2023

We would have to reuse returnStatus and that could work but getting response headers from xmlhttprequest is a nightmare. The code may looks similar but ParseServerRestController returns headers as a json, JS RestController returns headers as a single string that needs to be parsed.

@mtrezza
Copy link
Member

mtrezza commented Oct 3, 2023

Can this PR be closed thanks to parse-community/Parse-SDK-JS#2033?

@dplewis
Copy link
Member Author

dplewis commented Oct 3, 2023

We have to wait for parse-community/Parse-SDK-JS#2033 to be released for the test cases here to pass.

I've updated the PR to pass headers in the ParseRestController similarly to the JS RestController. The JS SDK will handle the headers.

@mtrezza
Copy link
Member

mtrezza commented Oct 4, 2023

That's a great fix; so should be able to merge this in Nov when we'll do a stable release of the JS SDK with that fix included.

@nebitrams
Copy link

Thank you @mtrezza, @dplewis and team for fixing this. Since I can use directAccess: false as a work around, I am happy to wait for the bug fix in Nov 2023.

@mtrezza
Copy link
Member

mtrezza commented Feb 15, 2024

parse-community/Parse-SDK-JS#2033 has been merged, but we cannot use the latest Parse JS SDK release in Parse Server due to #8818; so I believe we need fix #8818 first before we can merge this PR here.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

parse-community/Parse-SDK-JS#2033 has been merged and it's in the JS SDK that PS7 is using, but tests fail.

@dplewis
Copy link
Member Author

dplewis commented Apr 12, 2024

@mtrezza Tests have been updated. This is ready to merge.

@dplewis dplewis requested a review from mtrezza April 12, 2024 16:06
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link

codecov bot commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.16%. Comparing base (f1469c6) to head (708d12e).
Report is 6 commits behind head on alpha.

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8766      +/-   ##
==========================================
+ Coverage   94.13%   94.16%   +0.02%     
==========================================
  Files         186      186              
  Lines       14687    14724      +37     
==========================================
+ Hits        13826    13865      +39     
+ Misses        861      859       -2     

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

@mtrezza mtrezza merged commit 5b0efb2 into parse-community:alpha Apr 14, 2024
24 of 26 checks passed
parseplatformorg pushed a commit that referenced this pull request Apr 14, 2024
# [7.1.0-alpha.6](7.1.0-alpha.5...7.1.0-alpha.6) (2024-04-14)

### Bug Fixes

* `Parse.Cloud.startJob` and `Parse.Push.send` not returning status ID when setting Parse Server option `directAccess: true` ([#8766](#8766)) ([5b0efb2](5b0efb2))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 7.1.0-alpha.6

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Apr 14, 2024
@dplewis dplewis deleted the statusId-directAccess branch April 18, 2024 21:54
parseplatformorg pushed a commit that referenced this pull request Jun 30, 2024
# [7.1.0-beta.1](7.0.0...7.1.0-beta.1) (2024-06-30)

### Bug Fixes

* `Parse.Cloud.startJob` and `Parse.Push.send` not returning status ID when setting Parse Server option `directAccess: true` ([#8766](#8766)) ([5b0efb2](5b0efb2))
* `Required` option not handled correctly for special fields (File, GeoPoint, Polygon) on GraphQL API mutations ([#8915](#8915)) ([907ad42](907ad42))
* Facebook Limited Login not working due to incorrect domain in JWT validation ([#9122](#9122)) ([9d0bd2b](9d0bd2b))
* Live query throws error when constraint `notEqualTo` is set to `null` ([#8835](#8835)) ([11d3e48](11d3e48))
* Parse Server option `extendSessionOnUse` not working for session lengths < 24 hours ([#9113](#9113)) ([0a054e6](0a054e6))
* Rate limiting can fail when using Parse Server option `rateLimit.redisUrl` with clusters ([#8632](#8632)) ([c277739](c277739))
* SQL injection when using Parse Server with PostgreSQL; fixes security vulnerability [GHSA-c2hr-cqg6-8j6r](GHSA-c2hr-cqg6-8j6r) ([#9167](#9167)) ([2edf1e4](2edf1e4))

### Features

* Add `silent` log level for Cloud Code ([#8803](#8803)) ([5f81efb](5f81efb))
* Add server security check status `security.enableCheck` to Features Router ([#8679](#8679)) ([b07ec15](b07ec15))
* Prevent Parse Server start in case of unknown option in server configuration ([#8987](#8987)) ([8758e6a](8758e6a))
* Upgrade to @parse/push-adapter 6.0.0 ([#9066](#9066)) ([18bdbf8](18bdbf8))
* Upgrade to @parse/push-adapter 6.2.0 ([#9127](#9127)) ([ca20496](ca20496))
* Upgrade to Parse JS SDK 5.2.0 ([#9128](#9128)) ([665b8d5](665b8d5))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 7.1.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Jun 30, 2024
parseplatformorg pushed a commit that referenced this pull request Jun 30, 2024
# [7.1.0](7.0.0...7.1.0) (2024-06-30)

### Bug Fixes

* `Parse.Cloud.startJob` and `Parse.Push.send` not returning status ID when setting Parse Server option `directAccess: true` ([#8766](#8766)) ([5b0efb2](5b0efb2))
* `Required` option not handled correctly for special fields (File, GeoPoint, Polygon) on GraphQL API mutations ([#8915](#8915)) ([907ad42](907ad42))
* Facebook Limited Login not working due to incorrect domain in JWT validation ([#9122](#9122)) ([9d0bd2b](9d0bd2b))
* Live query throws error when constraint `notEqualTo` is set to `null` ([#8835](#8835)) ([11d3e48](11d3e48))
* Parse Server option `extendSessionOnUse` not working for session lengths < 24 hours ([#9113](#9113)) ([0a054e6](0a054e6))
* Rate limiting can fail when using Parse Server option `rateLimit.redisUrl` with clusters ([#8632](#8632)) ([c277739](c277739))
* SQL injection when using Parse Server with PostgreSQL; fixes security vulnerability [GHSA-c2hr-cqg6-8j6r](GHSA-c2hr-cqg6-8j6r) ([#9167](#9167)) ([2edf1e4](2edf1e4))

### Features

* Add `silent` log level for Cloud Code ([#8803](#8803)) ([5f81efb](5f81efb))
* Add server security check status `security.enableCheck` to Features Router ([#8679](#8679)) ([b07ec15](b07ec15))
* Prevent Parse Server start in case of unknown option in server configuration ([#8987](#8987)) ([8758e6a](8758e6a))
* Upgrade to @parse/push-adapter 6.0.0 ([#9066](#9066)) ([18bdbf8](18bdbf8))
* Upgrade to @parse/push-adapter 6.2.0 ([#9127](#9127)) ([ca20496](ca20496))
* Upgrade to Parse JS SDK 5.2.0 ([#9128](#9128)) ([665b8d5](665b8d5))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 7.1.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse.Cloud.startJob not returning jobStatusId
4 participants