vert-x3 / vertx-config

Vert.x Configuration Service
Apache License 2.0
54 stars 64 forks source link

store config docs appear out of date: `optional` property required on store config ? #43

Closed StephenOTT closed 6 years ago

StephenOTT commented 6 years ago

Working in JS with a YAML store,

A store must currently be configured with the optional property or vertx throws a null pointer exception

var yamlStore = {
  "type" : "file",
  "format" : "yaml",
  "config" : {
    "path" : yamlPath
  },
  "optional":false
}

ref: https://github.com/vert-x3/vertx-config/blob/master/vertx-config/src/main/java/io/vertx/config/ConfigStoreOptions.java#L49

gsson commented 6 years ago

This also causes my existing configurations to break since the attribute was added without a default fallback. Preferrably I'd have a default value if the attribute is missing, but I'd settle for adding a line in "Potentially breaking changes" in the change log at https://github.com/vert-x3/wiki/wiki/3.5.1-Breaking-Changes

cescoffier commented 6 years ago

Wow... yes. Definitely, something I should have mentioned in the list of breaking change. But, I believe it's somehow a bug too as optional should be false by default.

gsson commented 6 years ago

Yea, but ConfigStoreOptions is using this one;

  /**
   * Get the Boolean value with the specified key
   *
   * @param key  the key to return the value for
   * @return the value or null if no value for that key
   * @throws java.lang.ClassCastException if the value is not a Boolean
   */
  public Boolean getBoolean(String key) {
    Objects.requireNonNull(key);
    return (Boolean)map.get(key);
  }

Which will return null, which when casted to a primitive boolean will become an NPE. I guess the intention was to use optional = json.getBoolean("optional", false);?

StephenOTT commented 6 years ago

So what is the current fix for this?

StephenOTT commented 6 years ago

The .setoptional(false) does not even seem to work when using Java

cescoffier commented 6 years ago

I believe it has been fixed. Please reopen if you still have the issue.