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

Reorganization of ParseServerOptions #7069

Open
dblythy opened this issue Dec 14, 2020 · 20 comments
Open

Reorganization of ParseServerOptions #7069

dblythy opened this issue Dec 14, 2020 · 20 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Dec 14, 2020

Is your feature request related to a problem? Please describe.
As Parse Server continues to evolve, so do the Parse Server options.

Describe the solution you'd like
There are certain options that in my view should be grouped together for neater organization, such as:

User Options:
-accountLockout
-enableAnonymousUsers
-expireInactiveSessions
-passwordPolicy
-preventLoginWithUnverifiedEmail
-revokeSessionOnPasswordReset
-sessionLength
-userSensitiveFields

Database Options:
-allowClientClassCreation
-allowCustomObjectId
-collectionPrefix
-databaseOptions
-objectIdSize
-protectedFields

REST Options:
-allowHeaders
-allowOrigin
-enableExpressErrorHandler
-middleware

Adapters:
-analyticsAdapter
-auth
-cacheAdapter
-databaseAdapter
-emailAdapter
-filesAdapter
-loggerAdapter
-push

Keys:
-clientKey
-databaseURI
-dotNetKey
-encryptionKey
-fileKey
-javascriptKey
-masterKey
-readOnlyMasterKey
-restAPIKey
-webhookKey

Server Info:
-appId
-appName
-cloud
-cluster
-customPages
-directAccess
-host
-jsonLogs
-masterKeyIps
-maxLimit
-mountPath
-port
-publicServerURL
-serverURL

Cache:
-cacheMaxSize
-cacheTTL
-enableSingleSchemaCache
-schemaCacheTTL

Email Options:
-emailVerifyTokenReuseIfValid
-emailVerifyTokenValidityDuration
-verifyUserEmails

Graph QL:
-graphQLPath
-graphQLSchema
-mountGraphQL
-mountPlayground
-playgroundPath

Idempotency:
-idempotencyOptions

Live Query:
-liveQuery
-liveQueryServerOptions
-startLiveQueryServer

Logs:
-logLevel
-logsFolder
-maxLogFiles
-silent
-verbose

Files:
-maxUploadSize
-preserveFileName

Push:
-scheduledPush

Server Events:
-serverCloseComplete
-serverStartComplete

I don't think every option requires categorization, but I think it could be handy to have all similar options tied together and cleaner in the docs, such as:

email : {
  verifyUserEmails: true,
  emailVerifyTokenValidityDuration: 2 * 60 * 60,
  preventLoginWithUnverified: false,
  resetTokenReuseIfValid: false
}

Considerations:

-How to handle options that could be under 2 categories, such as emailAdapter, which could be under adapters and email options
-This change would either need to be backwards compatible, or be a breaking change requiring changes to existing index.js.

The goal of this would be to ensure consistency and maintainability for ParseServerOptions, and to prevent them from continuing to bloat.

Please let me know your thoughts.

@mtrezza
Copy link
Member

mtrezza commented Dec 15, 2020

Thanks for suggesting.

This would certainly be an improvement in terms of configuration structure and oversight.

However, as discussed in the past, this would have to be look at holistically as it would be a breaking change affecting virtually every Parse Server deployment. The change efforts required can vary greatly depending on the specific environment, automation complexity and setup in which Parse Server is deployed.

Internally, changed would have to be made in:

  • Parse Server config definition
  • test cases
  • docs

Ergo, the frequency of such changes should be kept as low as possible and any fundamental overhaul of the Parse Server configuration should be well crafted for the foreseeable future. Such a change may as well justify - or be part of - a Parse Server major version increment.

All this goes to say that just regrouping parameters may not be all that should be considered here, but also:

  • establish and document a naming syntax for new parameters
  • rename existing parameters according to the syntax
  • evaluate current parameters and their related features as to whether they should be combined, deprecated or refactored

@dblythy
Copy link
Member Author

dblythy commented Dec 15, 2020

Fair comments. It’s my view that the problem will continue to get worse, however I agree it’s hard to justify such a breaking change for such a nit.

Personally I think cacheOptions, GraphQLoptions and keys are probably the only ones that need better organisation.

@mtrezza
Copy link
Member

mtrezza commented Dec 15, 2020

I agree it’s hard to justify such a breaking change for such a nit

I think it justifiable, just needs to be looked at holistically. I think it should be taken on.

I would advice to first discuss the intend changed here in detail before creating a PR because this will require so many changes that the PR will likely need several rebases/merges and face merge conflicts.

I suggest this process:

  • define new syntax for parameters (node and env var naming, description text)
  • identify any outdated/refactorable parameters
  • suggest new config structure (+ env vars)
  • refactor description text for each parameter
  • present final config structure for community feedback
  • prepare docs change PRs
  • create parse server and SDK PRs
  • merge docs PRs

@Moumouls
Copy link
Member

i think the Parse Server Options need this kind of re work, and group of options proposed by @dblythy at first glance seems not bad at all.

But the breaking change is massive for parse developers and also for all of our internal code (tests, etc...)
I think this kind of rework need a deprecated strategy.

During one major version we need to support current options and the new one.

Also since changes could be huge and lead to massive PR. I would propose also a development process:

  1. Prepare a code base to support old Options and new Options

  2. Create a new-parse-server-options branch on parse-server

  3. Define an option group to support (ex: Database Options)

  4. Work on it (and continue to support current option, i think group name will not conflict)

  5. Fix tests

  6. Send a PR from working branch to new-parse-server-options

Do it again for each option group.
This way, we can contribute progressively to support the new Parse Server Options.

Finally when new-parse-server-options is ready, we can open the final PR to master and start showing some deprecated warnings when old ParseServerOptions are used, then old ParseServerOptions could be removed in next major version

What do you think about that ?

@mtrezza
Copy link
Member

mtrezza commented Dec 16, 2020

I'm supportive of slowly phasing out the old options while still supporting them alongside with the new options, if that does not complicate the code too much or introduce other costs.

This change has no functional implication, it is merely aesthetics, so the costs and risks for however this is done should be kept as low as possible.

Another strategy to consider may be introducing a config version parameter as part of the configuration. The parameter can default to the old config, but developers can set it to the new config and use the new structure. Internally, this only requires a translation method that translates the old config into the new config. It does not require multiple PRs or to change all tests at once. It is similar to AWS policy versioning but for Parse Server configuration:

{
   configVersion: 20210101,
   ...
}

We may benefit from this feature in the future, so the efforts are not for just a one time change.

@dblythy
Copy link
Member Author

dblythy commented Dec 16, 2020

So with the config version idea, Config.validate would translate according to the configVersion?

e.g,

configVersion 20210101 converts config.keys.masterKey to config.masterKey, so that the rest of the code base can still access config.masterKey

@mtrezza
Copy link
Member

mtrezza commented Dec 16, 2020

configVersion 20210101 converts config.keys.masterKey to config.masterKey, so that the rest of the code base can still access config.masterKey

Yes, along these lines, but that's just an idea, maybe that complexity is not needed after all.

@dblythy
Copy link
Member Author

dblythy commented Dec 16, 2020

I would imagine in terms of lines changed it would be the simplist change (rather than changing every single mention of config.masterKey), as all the current lines would stay the same. There would just need to be a function for transposing the keys depending on the version - which I don't think the logic is too complicated - and test cases depending on the configVersion. It would also be easy to make backwards compatible.

I'd imagine ENV variables would have to stay the same as they are, which I don't think will be too big of a deal.

@mtrezza
Copy link
Member

mtrezza commented Dec 16, 2020

Well, that's something we'd have to figure out. It could create more confusion if there is a discrepancy between the configuration structure on how to access the config internally. I think internally, the config should be adapted to the new structure, because it is more developer friendly, and the old config should be transposed to the internal new config. But again, this approach was just an idea, the effort may not be necessary.

@mtrezza
Copy link
Member

mtrezza commented Jan 30, 2021

Keys:
-clientKey
-databaseURI
-dotNetKey
-encryptionKey
-fileKey
-javascriptKey
-masterKey
-readOnlyMasterKey
-restAPIKey
-webhookKey

Just noticed, I don't think databaseURI fits in keys; also keys that belong to a distinct features such as fileKey or encryptionKey may fit better in that distinct feature group.

@sadortun
Copy link
Contributor

@mtrezza After working with https://www.npmjs.com/package/convict i think it could be a really good way to expose a configuration interface as it allow to define a configuration schema, and validation.

It also allow binding to ENV, and defining prod/dev/test configs

@mtrezza
Copy link
Member

mtrezza commented Mar 22, 2021

Interesting, maybe there are also other config definition concepts to consider. @sadortun can you give more details about what the benefits of convict would be and how this would practically work for Parse Server?

The Deprecator #7303 may help when changing Parse Server options.

@sadortun
Copy link
Contributor

sadortun commented Apr 8, 2021

Hi @mtrezza sorry for the delay, I'm trying to catch up on stuff that is accumulating !

One thing I like with Convict is that it does all the co fig validation, and handle multiple types of configs (yaml, JSON, ENV Vars....)

The format is also intuitive to use.

Currently, my guess is thatmost new projects start by cloning the parse-server-example, while the project is awesome to get a quick start, the config process could be improved.

I think all the configuration should be kept out of the main script into a separate file.

This would allow a few things:

  • lower the time to setup for new users by directing the user to exactly what they need to fill in to make the server start.
  • not having to understand all the server init process to get started
  • easely track when new options are added
  • show optional/mandatory parameters
  • defer the internal configuration validation to Convict which does a great job at it!

For example currently bellow the config you can find this comment:

// Client-keys like the javascript key or the .NET key are not necessary with parse-server
// If you wish you require them, you can set them as options in the initialization above:
// javascriptKey, restAPIKey, dotNetKey, clientKey

@mtrezza
Copy link
Member

mtrezza commented Apr 8, 2021

@sadortun I like the idea! Thanks for this insight.

@dblythy What do you think?

@dblythy
Copy link
Member Author

dblythy commented Apr 8, 2021

Any improvements to the parse-server-example project I'm always keen for. I think this change could make it a little easier for us to support developers in the forum and stackoverflow. Once we've seen Convict's implementation there, maybe we could consider using it to handle Parse's internal config validation.

@mtrezza
Copy link
Member

mtrezza commented Apr 8, 2021

How do you see this being used in the example repo?

@sadortun
Copy link
Contributor

sadortun commented Apr 8, 2021

@mtrezza I'm using it in a project, you can have a look here at:

@mtrezza
Copy link
Member

mtrezza commented Apr 9, 2021

2 thoughts:

  1. I am not sure that adding this solely to parse-server-example provides any benefit; it probably makes the example more complex by adding a technically unnecessary layer of abstraction between the (new) developer and Parse Server

  2. Considering this for Parse Server, we would need to look at the current config mechanism, which covers some things:

    • JS parameter definitions
    • Environment parameter definitions
    • Config file parameter import
    • Merging of JS, environment and config file parameters
    • Default value injection
    • Deprecation warning (it's a separate module, but hooked in between parameter import and default value injection)
    • Value validation
    • JSDoc creation
    • TS type / interface definitions

    The challenge would be to determine:

    • which parts of the current mechanism can be done by convict
    • how can convict be integrated into the current mechanism if it cannot substitute all parts
    • which benefits does convict provide over the current mechanism, and how much work would it be to simply add these benefits to the current mechanism instead of switching to convict vs. how much work would it be to add convict
    • alternatives to convict?

@sadortun
Copy link
Contributor

sadortun commented Apr 9, 2021

You are absolutely right. @mtrezza Convict would be more for parse-server-example

My thought on this is that your example project should be as easy and convenient to get started with.

Adding a bit of complexity is fairly OK if it allow users can simply take the example project to start their own projects.

@mtrezza
Copy link
Member

mtrezza commented Apr 9, 2021

Adding a bit of complexity is fairly OK if it allow users can simply take the example project to start their own projects.

  • The example project should already be "plug-and-play" or "deploy-and-run"; if multiple configs are needed, that is already possible with a config file, or simply a commented out section in index.js
  • Dependencies come with a management cost that has to be outweighed by the benefit they bring; we need to update for vulnerabilities, adapt for breaking changes, etc.
  • The example project's focus should be on essentials around Parse Server; convict is a subjective convenience; unless it becomes an integral part of Parse Server

I think convict could be a valuable addition to Parse Server though.

@sadortum, can you tell which features of Parse Server's current config mechanism convict already covers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

4 participants