stormpath namespace for stormpath config file

[lhazlewood]

In our current spec here, we do not prefix the properties with the stormpath namespace/key.

This causes problems when stormpath.yaml is read within the context of other YAML files. For applications that support yaml for config already, the above config should look like this instead:

stormpath:
  client:
    apiKey:
      file: null
      id: null
      secret: null
    cacheManager:
      defaultTtl: 300
      defaultTti: 300
      caches: { }
    baseUrl: "https://api.stormpath.com/v1"
    connectionTimeout: 30
    authenticationScheme: "SAUTHC1"
    proxy:
      port: null
      host: null
      username: null
      password: null
  application:
    name: null
    href: null

This allows you to take your Stormpath config and copy and paste it into existing yaml configs because the config is namespaced appropriately.

Any thoughts around this?

[nbarbettini]

This is just an artifact of this spec being older, I believe. It should absolutely be namespaced with stormpath.

I'm not sure if this will be a breaking change for libraries that have already implemented some config loading functionality? Since my .NET implementation is newer, it already expects it to be namespaced.

Tl;dr - yes, spec should change.

[lhazlewood]

Also, FWIW, this is already the model in the Java config land, and I don't want to change this. Every single config property is prefixed with stormpath. and it fits into idiomatic expectations.

[rdegges]

I'm not a fan of this at all. I think it adds unnecessary complexity.

EG: Doing this opens up a whole bunch of un-ideal side effects:

• Do our JSON configs when initializing a Client object manually also need to be prefixed with stormpath? If so, this is totally overkill and looks bad.
• If not, that means we're no longer consistent for developers: stormpath as the top-level key for all yml configs, but no stormpath for top-level JSON configs when initializing clients directly. Very odd.

IMO, if you wanna do this in Java / whatever because you already use other configs -- go for it, but IMO, it's not a good idea to introduce redundant naming stuff into places where it is already apparent.

[robertjd]

I agree that this file should have stormpath as the root node. This would make it map consistently with our environment variable pattern of STORMPATH_PROPERTY_PATH.

@rdegges Regarding your JSON concern: having Stormpath as the root node, in the YAML, doesn't *require• us to prefix with stormpath if it doesnt make sense. For example, with the proposed change I wouldn't change this configuration pattern for Express:

app.use(stormpath.init(app, {
  client: {
    // api key configuration
  },
  web: {
    // framework configuration
  }
}));

It would just be assume you're passing stormpath properties and our config parser would handle the object extension. Easy stuff. I view the proposed change as a simple sanity cleanup of our base configuration file, this does not dictate how a more specific constructor (such as a framework) will accept arguments.

[nbarbettini]

Agreed @robertjd. Passing an object graph to a client constructor should always assume an unstated root node of stormpath. The user doesn't have to specify that.

I'd propose that the parsers that load JSON/YAML files should be intelligent enough place everything into the stormpath namespace if no root node called stormpath exists. That way, people can write their configs either way.

[lhazlewood]

In node land, I presume you can just do this (pseudo-code):

var nested = config.stormpath
//use nested where you would have used config previously

This is a trivial change to ensure that our config can be used both inside of and outside of other yaml files without changing anything. Determinism and less conditional branching FTW! :)

[robertjd]

I'm looking at this again, and the original linked file of concern is here:

https://github.com/stormpath/stormpath-sdk-spec/blob/master/specifications/config.md#default-configuration

This appears to have stormpath as the root node/property, as suggested by this issue. So I think this issue can be closed?

[lhazlewood]

I'm pretty sure it can be closed.