InitialConfig second draft

[darkgl0w]

Address #1444

@mcollina > I followed your advices and now initialConfig uses a schema to validate what must be exposed, I also documented the functionality.

Let me know if you want that I make some rework.

PS: sorry again for the burden of re-opening a PR, I currently have a poor cellular connection and it was easier for me to do like this in order to avoid commit pollution 😸

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows Code of conduct

[mcollina]

Good work! I’ve just left a couple of nits.

[mcollina]

LGTM

[cemremengu]

Maybe we should put this in a separate file per #1475 ?

[mcollina]

@cemremengu it mostly is already, that was the whole of point of some of the latest comments.

[darkgl0w]

@mcollina I was looking at this while doing the backport for the 1.x branch.
I can litteraly remove the function from fastify.js, add this to the initialConfigValidation.js file

function validateInitialConfig (options) { // options instead of opts
  // We clone the initial configuration options object
  let opts = Object.assign({}, options) // add this here

  const isOptionValid = validate(opts)

and we require initialConfigValidation like this in fastify.js:

const getSecuredInitialConfig = require('./lib/initialConfigValidation')

is it what you had in mind?

[mcollina]

wait for this to land before backporting.

[mcollina]

Still LGTM ;).

[mcollina]

LGTM

[nwoltman]

Just 2 small things to clean up and then this will be good!

[nwoltman]

LGTM. Great job @darkgl0w!

[delvedor]

I feel that this solution is overkill. Why do we need to clone and freeze the object?

[nwoltman]

Still LGTM 👍

[mcollina]

@delvedor do you still strongly object to the deep-freeze?

[alex-ppg]

A minor comment, it appears that the schema defined for the options parameter does not contain complex data structures. As such, JSON.parse(JSON.stringify(obj)) will create a deep-clone of the object fast without needing the addition of an external dependency. I understand the flexibility that the rfdc package allows but would a configuration for the fastify server possess any complex data structures that should be exposed? Just a thought, I understand this has passed numerous reviews.

P.S. I think line 82 in lib/initialConfigValidation.js has a reduntant check, typeof value === 'object', which is covered by the preceding if clause.

[mcollina]

@alex-ppg unfortunately out opts could contain some complex data structures (streams, buffers) that would not like being stringified.

[mcollina]

Can you please add a test passing a stream to pino? So we can verify this mechanism is not causing issues with the stream.

[delvedor]

Hello! Sorry for the delay, I left a couple of comments.
I still think that deep clone and deep freeze is overkill, but I'll not block this.

[darkgl0w]

As suggested by @delvedor I refactored the default behavior when Fastify is initialized without options.

Possible enhancements :
Currently internal defaults are passed directly to the different functions that require them like this const requestIdHeader = options.requestIdHeader || 'request-id'.

What we can do, is define internal defaults inside constants declared at the top of fastify.js file like it is done for the bodyLimit that defaults to theconst DEFAULT_BODY_LIMIT = 1024 * 1024 (this can be done according to this on going work here #1475) .

Doing so we can expand the current function signature to something like this:

getSecuredInitialConfig(options, defaults)

and thus we could implement an automatic update of the schema defaults every time they are internally changed.

[darkgl0w]

WDYT if I export the deepFreezeObject() as an utils like this

module.exports.utils = { deepFreezeObject }

or like this:

module.exports.deepFreezeObject = deepFreezeObject

It actually bothers me a lot to not be able to achieve 100% code coverage on this file because due to the current filtering schema a path is actually not able to be taken (it's like an unpleasant pinch x)). Exporting the function I would be able to effectively completely test the way it should work and in addition it can be reusable if by chance a future object deep freeze should be done in another part of fastify internals.

Are you ok with this ? can I go for it ?

I already have a commit ready with all the changes needed and 100% code coverage achieved 😁 .

[mcollina]

I think it’s a great idea. Il giorno sab 9 mar 2019 alle 17:08 darkgl0w <[email protected]> ha scritto:

…

As suggested by @delvedor <https://github.com/delvedor> I refactored the default behavior when Fastify is initialized without options. Possible enhancements : Currently internal defaults are passed directly to the different functions that require them like this const requestIdHeader = options.requestIdHeader || 'request-id'. What we can do is define internals default inside constants declared at the top of fastify.js file like it is done for the bodyLimit that defaults to theconst DEFAULT_BODY_LIMIT = 1024 * 1024 (this can be done according to this on going work here #1475 <#1475>) . Doing so we can expand the current function signature to something like this: getSecuredInitialConfig(options, defaults) and thus we could implement an automatic update of the schema defaults every time they are internally changed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1471 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL4yqgxE_dtU4zmya-yHPLu3U8OFrdks5vU9x4gaJpZM4bKRcw> .

[mcollina]

Would you like to add getSecuredInitialConfig(options, defaults) support as well, or do that in another PR?

[darkgl0w]

I think it's better to do that in another PR, this way it will be cleaner and easier to review.

[mcollina]

@darkgl0w please send over the follow up PR!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.