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

Direct Access to parse-server #2316

Merged
merged 3 commits into from
Sep 9, 2016
Merged

Direct Access to parse-server #2316

merged 3 commits into from
Sep 9, 2016

Conversation

flovilmart
Copy link
Contributor

This PR enables direct access to parse-server, when using the JS SDK from the current node runtime.

All API calls from the JS-SDK are router through route matching (but /functions/:functionName)

Enable that feature by setting PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS=1

expect(pushStatus.get('status')).toBe('failed');
done();
});
}, 500);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to add some delay here as with DIRECT_ACCESS the status is not updated upon failure...

@flovilmart
Copy link
Contributor Author

Closing for now as will rebase when #2346 is merged

@flovilmart flovilmart closed this Jul 20, 2016
@andrewimm
Copy link
Contributor

This sounds like a great approach. I'll be sure to review when you reopen.

@flovilmart
Copy link
Contributor Author

If you have time to review #2346 I can get going, I'd like to have that merge pretty soon

@flovilmart flovilmart reopened this Jul 20, 2016
@ghost
Copy link

ghost commented Jul 20, 2016

@flovilmart updated the pull request.

@flovilmart
Copy link
Contributor Author

@andrewimm I just reopened it, and tweaked a few parts to make it nicer.

@codecov-io
Copy link

codecov-io commented Jul 20, 2016

Current coverage is 92.16% (diff: 90.83%)

Merging #2316 into master will increase coverage by 0.05%

@@             master      #2316   diff @@
==========================================
  Files           100        101     +1   
  Lines         12282      12358    +76   
  Methods        1522       1542    +20   
  Messages          0          0          
  Branches       2020       2035    +15   
==========================================
+ Hits          11313      11390    +77   
+ Misses          969        968     -1   
  Partials          0          0          

Powered by Codecov. Last update 5676a6f...e3098fc

@ghost
Copy link

ghost commented Jul 20, 2016

@flovilmart updated the pull request.

@ghost
Copy link

ghost commented Jul 20, 2016

@flovilmart updated the pull request.

@ghost
Copy link

ghost commented Aug 20, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @acinader as a potential reviewer. Could you take a look please or cc someone with more context?

@@ -3,55 +3,55 @@ var LoggerController = require('../src/Controllers/LoggerController').LoggerCont
var FileLoggerAdapter = require('../src/Adapters/Logger/FileLoggerAdapter').FileLoggerAdapter;

describe("Cloud Code Logger", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

just outta curiosity, why the change here? it looks to me like you are just swapping the order of two tests? or am i missing something else...

@acinader
Copy link
Contributor

The bot asked me to look, so I'm looking. Not really adding any value at pr's (yet), but good to read the code.

My big question on this one is "why?". Would be helpful to me, though not necessarily helpful to the code review, if you could give me a use case to understand what is going on.

query = data;
}

logRequest("internal"+path, method, data, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

add ' ' around +, or better yet, use template

@acinader
Copy link
Contributor

ok, i reviewed it, but since i don't really understand what is going on, its only of value to me, not you ;).

@zleman1593
Copy link

@flovilmart Are you planning on merging this soon? Looking forward to it.

@flovilmart
Copy link
Contributor Author

Not soon, it needs some rebase as well as fine tuning for cloud code usage. Mostly a custom UserController for the Parse SDK.

@flovilmart
Copy link
Contributor Author

@andrewimm if you got a minute to check that out again, see if there is anything that could be improved here.

@ghost
Copy link

ghost commented Sep 3, 2016

@flovilmart updated the pull request - view changes

@ghost
Copy link

ghost commented Sep 3, 2016

@flovilmart updated the pull request - view changes

@flovilmart
Copy link
Contributor Author

flovilmart commented Sep 3, 2016

Fixes #1495

When using the DirectRestController, the sessionToken won't be created when saving a new user.

@acinader
Copy link
Contributor

acinader commented Sep 6, 2016

@flovilmart I did get around to getting set up and running the alpha tag, but I haven't had a chance to run through some of the account creation, editing, cloud code, tests that I want to do. Another day or two. But no issues in my very casual testing this weekend.

@flovilmart
Copy link
Contributor Author

flovilmart commented Sep 6, 2016

Alright that's good to hear! As long as you don't use Parse.User.currentUser() and Parse.Cloud.useMasterKey() that should be fine.

On another note, the installationId for the requests are 'cloud' so we don't generate sessionTokens. I believe it should have very little effet on your code.

Or maybe I should support passing installationId as a param. I'm not sure. @andrewimm what do you think?

@flovilmart
Copy link
Contributor Author

@acinader any update on your side?

@drew-gross
Copy link
Contributor

If this passes tests I'd be fine with skipping the experimental flag stage. We have a pretty comprehensive test suite, it would be pretty tough to make an implementation that passes tests but still has major issues.

@flovilmart flovilmart merged commit cb7b549 into master Sep 9, 2016
@flovilmart flovilmart deleted the DirectRESTController branch September 9, 2016 19:18
@acinader
Copy link
Contributor

acinader commented Sep 9, 2016

👍 we ran two releases through qa with alpha on our dev boxes. one with direct on and one with it off. I wanted to do better any profiling to see impact, but that's still gotta wait.

@flovilmart sorry for delay.

@flovilmart
Copy link
Contributor Author

It's gonna be in the next release, 2.2.19, still marked as experimental until we have a better idea on how to make sure it works 💯

So you'll be able to go back to the regular release cycle with the experimental flag on

@listingboat
Copy link

@flovilmart So, this is in the current version as experimental. I turned on PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS on our Heroku instance and saw faster performance, but also some issues. Our app is a cloud code web app hosted on Heroku.

I noticed in above comments:
"As long as you don't use Parse.User.currentUser() and Parse.Cloud.useMasterKey() that should be fine."

Why is that? We do use Parse.Cloud.useMasterKey to update Users. I can dig further if needed.

@flovilmart
Copy link
Contributor Author

you should not use Parse.Cloud.useMasterKey() at all.

Instead use:

user.save(null, {useMasterKey: true})

@nitrag
Copy link

nitrag commented Mar 31, 2017

What's the status on this feature in terms of it becoming the standard? I don't see any recent Issues. Is it safe to say it's production ready?

@flovilmart
Copy link
Contributor Author

We need to revamp some tests but yes, we can probably make it mainline.

@mtrezza
Copy link
Member

mtrezza commented Apr 21, 2020

This looks like a PR that would give a significant performance improvement without any breaking changes.

@acinader This is feature flagged in master, but not currently further looked at and not considered save for production, right?

@acinader
Copy link
Contributor

I'm the wrong person to ask. I don't use it in production. @dplewis or @davimacedo might have a better idea of what is outstanding to make this the default.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

I just realized that there is a newer thread from 2019. #5442 I'll ask from there.

@mtrezza mtrezza mentioned this pull request Apr 22, 2020
6 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.

9 participants