Skip to content

Implementing GET /config and POST /config support #283

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

Merged
merged 20 commits into from
Feb 11, 2016

Conversation

theill
Copy link
Contributor

@theill theill commented Feb 7, 2016

Adds support for retrieving global configuration by handling GET requests for /config. This will return a JSON payload with an params attribute with config.

Furthermore support for doing a POST to /config for updating this value.

@@ -75,6 +78,7 @@ function classNameIsValid(className) {
className === '_Session' ||
className === '_SCHEMA' || //TODO: remove this, as _SCHEMA is not a valid class name for storing Parse Objects.
className === '_Role' ||
className === '_GlobalConfig' ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't really go here, as this is the place for the names of classes that the client is allowed to create. _SCHEMA is only here for legacy reasons, it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but as I see it it's used by ExportAdapter (see https://github.com/theill/parse-server/blob/master/ExportAdapter.js#L63) to filter what collections you're able to query. Is it because I'm going through wrong path?

Copy link
Contributor

Choose a reason for hiding this comment

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

ExportAdapter is the legacy user of this class that needs to be fixed before we can remove _SCHEMA from the list of valid class names. Take a look at schemas.js for how do it without using the classNameIsValid function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll take a look. It seems to be schemas.js uses req.config.database.collection to access a collection which eventually ends up with classNameIsValid too but I'll dig a bit deeper tonight.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you are right. Maybe we need a database.rawCollection() function or something like that, to bypass the classNameIsValid check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will work but then we need one for update too because the current update also uses database.collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or I can just do rawCollection().findAndModify() on it if that's ok. I'll update my PR with that and wait for your comments.

@facebook-github-bot
Copy link

@theill updated the pull request.

@@ -0,0 +1,49 @@
// run test when changing related file using
// $ TESTING=1 node_modules/jasmine/bin/jasmine.js spec/ParseGlobalConfig.spec.js
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one of our team members is in the middle of making some changes to our testing framework, so this comment is probably going to become out of date soon. Can you remove it? We'll keep instructions for how to run tests in CONTRIBUTING.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, submitted patch 7733ab9

@facebook-github-bot
Copy link

@theill updated the pull request.

Updated tests accordingly to changed access
@facebook-github-bot
Copy link

@theill updated the pull request.

@facebook-github-bot
Copy link

@theill updated the pull request.

@@ -60,6 +60,9 @@ defaultColumns = {
"expiresAt": {type:'Date'},
"createdWith": {type:'Object'},
},
_GlobalConfig: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This defaultColumns object only tracks default columns for collections that contain Parse Objects (thats what _SCHEMA isn't in here. So I think config probably shouldn't be here either.

@drew-gross
Copy link
Contributor

Looking awesome! As soon as this is hidden behind the experimental flag, I can land this. We just don't want to get into a situation where we are shipping a new version on npm that includes in-progress features, like we accidentally did for schemas API :) Thanks for the great work!

@facebook-github-bot
Copy link

@theill updated the pull request.

@facebook-github-bot
Copy link

@theill updated the pull request.

@facebook-github-bot
Copy link

@theill updated the pull request.

@facebook-github-bot
Copy link

@theill updated the pull request.

@facebook-github-bot
Copy link

@theill updated the pull request.

drew-gross added a commit that referenced this pull request Feb 11, 2016
Implementing GET /config and POST /config support
@drew-gross drew-gross merged commit 5fb015e into parse-community:master Feb 11, 2016
@douineauromain
Copy link

Hello

So /1/config is now available ?
If now I install parse-server from npm will I have /1/config ?

If yes I think you should update https://parse.com/docs/server/guide#migrating

Thanks

@drew-gross
Copy link
Contributor

Config is still experimental. We are doing further testing and will update the docs once we are confident in the implementation and its compatibility with Parse.com.

@sghamaty
Copy link

Anyone know if this Parse Config addition is included in the 2.0.8 parse-server npm? If not, any ideas when it might get packaged up?

I just pushed 2.0.8 to the Heroku server and still got "Cannot GET /1/config"

Thanks

@gfosco
Copy link
Contributor

gfosco commented Feb 16, 2016

@sghamaty Did you change PARSE_MOUNT=/1 ? If not, the proper path is /parse/config by default.

@sghamaty
Copy link

Yes, my PARSE_MOUNT=/1 . Still not working.

On Tue, Feb 16, 2016 at 3:41 PM, Fosco Marotto notifications@github.com
wrote:

@sghamaty https://github.com/sghamaty Did you change PARSE_MOUNT=/1 ?
If not, the proper path is /parse/config by default.


Reply to this email directly or view it on GitHub
#283 (comment)
.

@drew-gross
Copy link
Contributor

You also need to set PARSE_EXPERIMENTAL_CONFIG_ENABLED=1 environment variable.

@sghamaty
Copy link

Yes, that did it. Thanks

You also need to set PARSE_EXPERIMENTAL_CONFIG_ENABLED=1 environment variable.

@douineauromain
Copy link

Is it possible to do something like
var api = new ParseServer({ databaseURI: 'mongodb://localhost:27017/dev', [...] parseExperimentalConfigEnabled: true });
?

@gfosco
Copy link
Contributor

gfosco commented Feb 26, 2016

@douineauromain It's not, but a PR that added this in src/index.js would be welcomed.

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