avaje / avaje-config

Application configuration / properties loading for JVM applications
https://avaje.io/config
Apache License 2.0
47 stars 7 forks source link

Potential NPE in CoreConfigurationBuilder #143

Closed agentgt closed 2 months ago

agentgt commented 2 months ago

So I know Avaje does not use the newer TYPE_USE aka JSpecify-like annotations yet but you still promise to respect API and thus API has to be annotated @Nullable.

  @Override
  public Configuration.Builder load(String resource) {
    final var configParser = parser(resource);
    try {
      try (var inputStream = resourceLoader.getResourceAsStream(resource)) {
        putAll(configParser.load(inputStream)); //  <-- this is bad inputStream can be null.
        return this;
      }
    } catch (IOException e) {
      throw new UncheckedIOException(e);
    }
  }
public interface ConfigParser {
  /**
   * Parse content into key value pairs.
   *
   * @param is configuration contents
   * @return Key-Value pairs of all the configs
   */
  Map<String, String> load(InputStream is); // <-- is implicitly NonNull
}
public interface ResourceLoader {

  /**
   * Return the InputStream for the given resource or null if it can not be found.
   */
  @Nullable
  InputStream getResourceAsStream(String resourcePath);
}

The above is why I don't use JSR 305 and something stronger like Checkerframework.

I would consider seriously switching to it in the long run given I could find something like this just glancing. (I'm not trying to be rude it just is a reality).

EDIT funny I'm not even sure if it will fail at the try eagerly but still its a NPE that should not happen.

EDIT it does not fail at the try as obviously you can have null which for some reason I was not expecting but makes sense.

rbygrave commented 2 months ago

Well, I think it's more that ConfigParser is missing the @NonNullApi ... and that cascades to there being that missing null check on the inputStream. So adding that in shows us that issue. Should actually move the @NonNullApi to the package level (but yeah a bit of time pressure when extracting the ConfigParser interface and that got missed).