Skip to content

Use context header to accept context instead of modifying body #7438

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

Closed
3 tasks done
cbaker6 opened this issue Jun 20, 2021 · 16 comments · Fixed by #7437
Closed
3 tasks done

Use context header to accept context instead of modifying body #7438

cbaker6 opened this issue Jun 20, 2021 · 16 comments · Fixed by #7437
Labels
state:released Released as stable version state:released-beta Released as beta version type:feature New feature or improvement of existing feature

Comments

@cbaker6
Copy link
Contributor

cbaker6 commented Jun 20, 2021

New Feature / Enhancement Checklist

Current Limitation

Currently, context can only be used by injecting a _context key/value into the body when using REST. This is problematic for SDK's that encode objects without modifying the body like the Swift SDK. This was originally discussed on community.parseplatform.org.

Feature / Enhancement Description

Currently, context is setup to be accepted in the header, but doesn't have an actual header:

export function handleParseHeaders(req, res, next) {
var mount = getMountForRequest(req);
var info = {
appId: req.get('X-Parse-Application-Id'),
sessionToken: req.get('X-Parse-Session-Token'),
masterKey: req.get('X-Parse-Master-Key'),
installationId: req.get('X-Parse-Installation-Id'),
clientKey: req.get('X-Parse-Client-Key'),
javascriptKey: req.get('X-Parse-Javascript-Key'),
dotNetKey: req.get('X-Parse-Windows-Key'),
restAPIKey: req.get('X-Parse-REST-API-Key'),
clientVersion: req.get('X-Parse-Client-Version'),
context: {},
};

Adding a new header X-Parse-Cloud-Context will address the issue.

Example Use Case

Using REST:

const req = request({
method: 'POST',
url: 'http://localhost:8378/1/classes/TestObject',
headers: {
'X-Parse-Application-Id': 'test',
'X-Parse-REST-API-Key': 'rest',
'X-Parse-Context': '{"key":"value","otherKey":1}',
},
body: {
foo: 'bar',
},
});

Alternatives / Workarounds

No workarounds for using context in the header. Will have to modify the body with _context like the other SDKs.

3rd Party References

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

What is the bigger picture here? Do we want to change all SDKs to transmit the context in the header?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 20, 2021

Can you find a more specific name? X-Parse-Context sounds ambiguous, because "context" can have many meanings.

What about X-Parse-Object-Context? I was thinking "Parse-Context" would be specific enough for parse objects/queries/etc.

What is the bigger picture here? Do we want to change all SDKs to transmit the context in the header?

Not sure, the header way is better IMO, but if the SDK's are injecting _context in the body, the server already supports this. I don't believe all/most of the SDKs support context at all. IMO, all SDKs should add the feature using the header (this PR). This also may be easier than modifying the body when it comes to implementing in the SDKs.

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

What about X-Parse-Object-Context? I was thinking "Parse-Context" would be specific enough for parse objects/queries/etc.

The context is actually not an "object" context but a Cloud Code Trigger context. What is passed in the context is made available in the triggers. I can't think of any extension beyond triggers, but if X-Parse-Trigger-Context sounds too narrow, maybe X-Parse-Cloud-Context or something similar. This is just in case we need another header that contains the word "context" in the future.

This also may be easier than modifying the body when it comes to implementing in the SDKs.

I would agree. I am not sure why I implement this as part of the body, but a comment from back then makes me think it was for a reason:

The context is passed along with the REST command, that is inside Parse.Object as a private property (for lack of a better way), but only to be removed from the object again upon arrival at Parse Server.

I'll see if I can find out the reason. Also see this comment.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 20, 2021

The context is actually not an "object" context but a Cloud Code Trigger context. What is passed in the context is made available in the triggers. I can't think of any extension beyond triggers, but if X-Parse-Trigger-Context sounds too narrow, maybe X-Parse-Cloud-Context or something similar. This is just in case we need another header that contains the word "context" in the future.

X-Parse-Cloud-Context sounds good to me. Let me know if you want me to make this change.

The context is passed along with the REST command, that is inside Parse.Object as a private property (for lack of a better way), but only to be removed from the object again upon arrival at Parse Server.

It seems like a small oversight to me since context was added to the info header check. The only problem I can see if SDKs had issues encoding JSON objects to strings on the client side when making headers, but I doubt this is true since most of them use REST anyways and are encoding everything else to JSON strings.

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

Yes, X-Parse-Cloud-Context seems good.

About the implementation, let's see...

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 20, 2021

One argument for transmitting it in the body instead of header could be the size limitations of headers. RFC does not impose a size limitation, but practically the limit in most implementations is much lower for header than for body.

If there's no size limitation then what's the problem? Also, if there are practical size limitations on specific deployments then the simple solution is to document it saying, "be sure to respect your actual header limits". That should be left to the developer to control. Either way, the PR doesn't jeopardize the previous way (as nothing was deleted) and only provides a way for SDKs who don't want to hack the body as I mentioned before.

Then, im Parse Server, the context was simply extracted from the request body (original commit, not current version):
https://github.com/mtrezza/parse-server/blob/51010fc33bfbbf82e832078f560a0a1b25c6ff28/src/RestWrite.js#L56-L60

This is hacky and not good code IMO. Adding to private vars and removing looks ugly and seems unmanageable. Not to mention, doing things like this prevent the developer from using such keys in their objects.

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

Not to mention, doing things like this prevent the developer from using such keys in their objects.

"Internal" keys with leading underscore are a concept throughout parse server, so that couldn't happen.

But you are right, it seems hacky. I think the issue is that Parse Server uses the whole HTTP body JSON object as a Parse Object definition, not allowing any "meta" info to be passed. That looks like a design flaw. It seems to make sense to transmit it as a header. I think however we should use one way of specifying the context and gradually change the other SDKs to use the header too.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 20, 2021

I think however we should use one way of specifying the context and gradually move the other SDKs to use the header too.

Using one way makes it easier to manage, but of course would require multiple PR's and knowledge across all of the SDKs to implement the change. Currently multiple private vars have header counterparts:

Header:

export function handleParseHeaders(req, res, next) {
var mount = getMountForRequest(req);
var info = {
appId: req.get('X-Parse-Application-Id'),
sessionToken: req.get('X-Parse-Session-Token'),
masterKey: req.get('X-Parse-Master-Key'),
installationId: req.get('X-Parse-Installation-Id'),
clientKey: req.get('X-Parse-Client-Key'),
javascriptKey: req.get('X-Parse-Javascript-Key'),
dotNetKey: req.get('X-Parse-Windows-Key'),
restAPIKey: req.get('X-Parse-REST-API-Key'),
clientVersion: req.get('X-Parse-Client-Version'),
context: {},
};

Private/Underscore keys:

if (
req.body &&
req.body._ApplicationId &&
AppCache.get(req.body._ApplicationId) &&
(!info.masterKey || AppCache.get(req.body._ApplicationId).masterKey === info.masterKey)
) {
info.appId = req.body._ApplicationId;
info.javascriptKey = req.body._JavaScriptKey || '';
delete req.body._ApplicationId;
delete req.body._JavaScriptKey;
// TODO: test that the REST API formats generated by the other
// SDKs are handled ok
if (req.body._ClientVersion) {
info.clientVersion = req.body._ClientVersion;
delete req.body._ClientVersion;
}
if (req.body._InstallationId) {
info.installationId = req.body._InstallationId;
delete req.body._InstallationId;
}
if (req.body._SessionToken) {
info.sessionToken = req.body._SessionToken;
delete req.body._SessionToken;
}
if (req.body._MasterKey) {
info.masterKey = req.body._MasterKey;
delete req.body._MasterKey;
}
if (req.body._context && req.body._context instanceof Object) {
info.context = req.body._context;
delete req.body._context;
}
if (req.body._ContentType) {
req.headers['content-type'] = req.body._ContentType;
delete req.body._ContentType;
}

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

That is interesting, so if any of these are specified in the body and in the header, the body overrides the header?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 20, 2021

That is interesting, so if any of these are specified in the body and in the header, the body overrides the header?

I believe so, from my interpretation of the code. This also showed when I was debugging the fix.

@mtrezza
Copy link
Member

mtrezza commented Jun 20, 2021

Then I take back my previous comment, because overriding header via body can be considered expected behavior / a feature. It seems your PR was the correct approach after all.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 21, 2021

It seems the only SDKs that have the ability to use context are the JS and Swift SDKs. I’ve checked the obj-c, android, flutter, pop, and dotNet and can’t find context in any of them.

The JS SDK uses the _ body modifications instead of using headers: https://github.com/parse-community/Parse-SDK-JS/blob/e6f15fdc09429a2c456ec6387d365914af8a8988/src/RESTController.js#L215-L295

@mtrezza
Copy link
Member

mtrezza commented Jun 21, 2021

I also looked into php, because acinader mentioned he would add this to the sdk, but it doesn't seem to be there.

The JS SDK uses the _ body modifications instead of using headers

I think we concluded that - as with other fields - both should be possible, with body overriding the header, correct?

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jun 21, 2021

I think we concluded that - as with other fields - both should be possible, with body overriding the header, correct?

Yes, both are still possible and both are still tested

@cbaker6 cbaker6 closed this as completed Oct 17, 2021
@parseplatformorg
Copy link
Contributor

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

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
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-beta Released as beta version type:feature New feature or improvement of existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants