Inconsistency with builder and initial loader. Default resource loader does not use the System class loader?

[agentgt]

InitialLoader / InitialLoaderContext does several things different than what the Configuration.builder does that are very hard to replicate with the builder.

For example let us say I want to replicate most of the default initialization:
One them is this:

Configuration.builder()
.load("application.properties")
.load("application-test.properties")
.build();

There are several problems with the above. I can't specify a maybeLoad and or fallbacks. I also had the surprise that the default resourceLoader does not do:

ClassLoader.getSystemResourceAsStream which is done in io.avaje.config.InitialLoadContext:

  private InputStream resourceStream(String resourcePath) {
    InputStream is = resourceLoader.getResourceAsStream(resourcePath);
    if (is == null) {
      // search the module path for top level resource
      is = ClassLoader.getSystemResourceAsStream(resourcePath);
    }
    return is;
  }

It seems there is a lot of duplication between the builder and InitialLoadContext.

The above I would expect:

  private InputStream resourceStream(String resourcePath) {
    return resourceLoader.getResourceAsStream(resourcePath);
  }

And the default resourceLoader to handle the ClassLoader.getSystemResourceAsStream(resourcePath) fallback.

[agentgt]

Also this:

avaje-config/avaje-config/src/main/java/io/avaje/config/InitialLoadContext.java

Line 102 in 6915e81

throw new IllegalStateException(e);

Should throw either UncheckedIOException or ignored (as in the file no longer exists).

EDIT that whole code block looks like duplication of stuff that Configuration.builder should do or know about:

  InputStream resource(String resourcePath, InitialLoader.Source source) {
    InputStream is = null;
    if (source == InitialLoader.Source.RESOURCE) {
      is = resourceStream(resourcePath);
      if (is != null) {
        loadedResources.add("resource:" + resourcePath);
      }
    } else {
      File file = new File(resourcePath);
      if (file.exists()) {
        try {
          is = new FileInputStream(file);
          loadedResources.add("file:" + resourcePath);
          loadedFiles.add(file);
        } catch (FileNotFoundException e) {
          throw new IllegalStateException(e);
        }
      }
    }
    return is;
  }

Consequently when you use the Configuration.builder unlike the default loading:

The source information is missing in CoreEntry and is the info is just initial.

That is I expect:

var c = Configuration.builder().load("blah.properties").build();
assertEquals("resource:blah.properties",m c.entry("property_in_blah").orElseThrow().source());

But that is not the case. See #139

[agentgt]

Not that it is super important but calling the ServiceLoader even just once is actually a significant cost.

That is a better solution is actually to have ResourceLoader, ConfigurationLog, ModificationEventRunner all extend some parent interface and then make one call to the ServiceLoader with the parent interface. Of course this gigantic breaking change for those that have implemented services already but just something to keep in mind for a major version change.

  private static ResourceLoader initialiseResourceLoader() {
    return ServiceLoader.load(ResourceLoader.class)
      .findFirst()
      .orElseGet(DefaultResourceLoader::new);
  }

  private ConfigurationLog initLog() {
    if (configurationLog == null) {
      configurationLog = ServiceLoader.load(ConfigurationLog.class)
        .findFirst()
        .orElseGet(DefaultConfigurationLog::new);
    }
    return configurationLog;
  }

  private ModificationEventRunner initRunner() {
    if (eventRunner == null) {
      eventRunner = ServiceLoader.load(ModificationEventRunner.class)
        .findFirst()
        .orElseGet(CoreConfiguration.ForegroundEventRunner::new);
    }
    return eventRunner;
  }

[SentryMan]

ServiceLoader even just once is actually a significant cost.

Well now, I was actually thinking recently on how I could make things faster. It seems I've got my work cut out for me.

[agentgt]

@SentryMan Hey aren't you on vacation? Go back to enjoying that 😄

Also I owe it to you and @rbygrave to be tasked with things so let me know if you want me to do anything.

[rbygrave]

I can't specify a maybeLoad and or fallbacks.

It does now due to #143

I can't specify ... fallbacks.

I don't know what you mean by that. Maybe you are referring to ClassLoader.getSystemResourceAsStream.

ClassLoader.getSystemResourceAsStream

Yes that can be pushed down into the DefaultResourceLoader. Done now via #146

Should throw either UncheckedIOException

Done via #142

Consequently when you use the Configuration.builder unlike the default loading: The source information is missing in CoreEntry and is the info is just initial.

Ok, can have a look. Edit: Going with #147 for now, I'll have another look later.

[rbygrave]

Released as version 3.15-RC1, you can try that and hopefully that addresses most of the points above.

[agentgt]

I don't know what you mean by that. Maybe you are referring to ClassLoader.getSystemResourceAsStream.

Oh what I mean is builder.load(File|Resource) sort of implies that it will fail if it can't find it which is indeed what happened albeit a NPE.

Now I can see builder.load is more like what I was calling a maybeLoad as in if the file/resource is not there we just ignore trying to load it.

I think I'm fine with this but we should update the Configuration.Builder#load javadoc to explicitly state that (if it doesn't find it an error will not happen).

In my ancient internal config library that we use in my company we have an enum of REQUIRE and ALLOW_MISSING or something. I think I really like how optionally loading is the only option because if it was really required like say in command line app you would probably do the checking yourself instead of delegating to avaje.

TLDR I like the changes just need some javadoc updates.

[rob-bygrave]

maybeLoad

An alternative would be to add a loadMaybe(...) that takes resource and file and for the load(...) methods to fail on missing resource.

This might be preferrable in that:

• It's probably more obvious for people using the builder
• It supports the case where people do want it to fail when a resource/file is missing

If we make that change, is loadMaybe(...) a good enough and obvious enough name?

Sticking with the single load(...) methods to me almost implies that 99% of the time missing resources are ok and that having both load(...) and loadMaybe(...) is less desirable (in a "less is more" kind of way).

Hmmm.

Edit: the opposite style is to add a loadRequired(resource|file) that fails on missing resource

[rbygrave]

Hi @SentryMan , when you are back if you could just check if you are good with leaving this as a single load() method with "loadMaybe" semantics (not throw an exception if the resource is missing).

If you are good with that and we don't want to change to add a loadMaybe() or a loadRequires() then we could probably release this next version.

[SentryMan]

I'm good with it, but I'd let it sit in RC a little more in case we cook up more ideas.

[rbygrave]

Yeah cool. This RC has bug fixes in it though so I don't want to sit on it too long.

[agentgt]

@rbygrave Sorry for the delay on the response. The only reason why I can see making
load() = loadRequire() is because prior to the RC release it essentially was by accident 😆 .

So I don't know if that requires bumping a major for you... these days I have no idea what version policy folks are doing (I'm not sure if you saw my reddit post about semver).

I looked at our in house configuration library (one day I will just open source it so you and @SentryMan can laugh at the over engineering) and it appears we rarely do REQUIRE.

The only time REQUIRE API wise was used was to load defaults sort of similar to typesafe config reference.conf but with an explicit name (typesafe in theory does not need a require because it loads all sources from the classpath or another words multiple reference.conf).

However our config lib has a similar mechanism to load.properties but where you could say it is required.

Honestly it added a whole bunch of complexity such that we created an enum called LoadFlag and if we went down the path of making multiple load given my experience I would recommend a similar pattern.

Just to give an idea of the disturbing level complexity you can get with LoadFlags:

public enum LoadFlag {
	NOT_REQUIRED,
	NOT_EMPTY,
	NO_OVERRIDE,
	FORCE,
	NO_REPLACE_EXISTING_KEY_VALUE,
	NO_ADD_KEY_VALUES,
	STOP_ON_FIRST_FOUND,
	NO_LOAD_CHILDREN,
	NO_RESOURCE_PROVIDERS,
	NO_DISCOVER_SERVICE_PROVIDERS,
	NO_FINAL_INTERPOLATION,
	NO_INTERPOLATION,
	NO_LOGGING,
	NO_RELOAD;
...
}

[agentgt]

So I'm good with load being optional.

The only thing I think that would seal the deal for me is to make load doing some logging at say TRACE on build or InitialLoader.

The reason is if one wants to figure out if a darn resource has loaded or not (see above we have NO_LOGGING as it does it by default which is good and REQUIRED was default when it should not have been).

[rob-bygrave]

That is excellent insight / feedback - thanks!!

... an idea of the disturbing level complexity you can get with LoadFlags

Yes, re-enforces the idea that we should try to keep it as simple as we can etc.

So I'm good with load being optional.

Great.

... load doing some logging at say TRACE on build or InitialLoader.

Lets add some logging there - I agree that is a real PITA when that is missing and people just want to know what was loaded.

requires bumping a major

I'm happy to bump the major for this. For me, I think this boils down to this change in behaviour of load() being considered a breaking behaviour change and I think that is fair enough.

[SentryMan]

If we're bumping a major might as well add that service loader change

[agentgt]

What JDK version is avaje config compiled?

If 17 @SentryMan parent the class that is to be registered as the one and only SPI a sealed class like I did here: https://jstach.io/rainbowgum/io.jstach.rainbowgum/io/jstach/rainbowgum/spi/RainbowGumServiceProvider.html

EDIT I see JDK 11 as the min so I guess no sealed class.

[agentgt]

@SentryMan sorry for the sporadic comments (its post work and on and off). But I forgot to add the ServiceLoader is like a memoized supplier. That is it is cached. Something to be aware of if you want to reduce resource loading.

[rob-bygrave]

We have got the ServiceLoader change in now so that completes all the tasks mentioned here. So now I think it's just a matter of confirming we are all good with 4.0-RC1 (released last night) and look to release 4.0.

[agentgt]

@rob-bygrave I will put the change in version (RC) on rainbowgum to try it out today. Thanks!

[SentryMan]

How we looking?

[rob-bygrave]

Just to say 4.0 is actually released. I assumed we were all good and released it (quite a few days ago now).