Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add URI Loader SPI #176

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft

Add URI Loader SPI #176

wants to merge 30 commits into from

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Sep 20, 2024

  • Adds a URI loader SPI for loading config from custom URIs
  • delete the Parser Class (it was basically just a map)

this is what I got so far, what do you think @agentgt?

Resolves #175

@agentgt
Copy link

agentgt commented Sep 21, 2024

Let me follow up and inspect more on Monday. It is looking good though!

Copy link

@agentgt agentgt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor critiques otherwise looks good.

avaje-config-toml/src/main/java/module-info.java Outdated Show resolved Hide resolved
avaje-config/src/main/java/io/avaje/config/EnvLoader.java Outdated Show resolved Hide resolved
@SentryMan SentryMan self-assigned this Sep 23, 2024
Copy link

@agentgt agentgt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of my critique is debatable so I'll let you or Rob decide.

@SentryMan SentryMan requested a review from agentgt September 28, 2024 22:50
Copy link

@agentgt agentgt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good enough to be pushed into master for me provided #177 changes get in first.

@SentryMan SentryMan marked this pull request as ready for review October 7, 2024 13:55
@rob-bygrave
Copy link
Contributor

Hmmm, not enthused by this PR. Let's start from the very top with the motivation. What are some actual examples of a URIConfigLoader? I've seen some things like env:// but that is very unconvincing to me because avaje-config already handles environment variables, so what exactly are some URIConfigLoader implementations that are intended to exist?

@agentgt
Copy link

agentgt commented Oct 9, 2024

I talk about some of the URI we use here : #175 (comment)

ultimately it is going to depend how opinionated you want to keep ajave.

The initial loading of avaje can all be expressed in URI. We do special loading:

  • system properties
  • env variables
  • profiles
  • some default resources
  • etc

In our config framework only one classpath resource is loaded and all that other shit is URIs for load.properties this includes cloud resources and k8s.

I’ll add more tomorrow.

@rbygrave
Copy link
Contributor

rbygrave commented Oct 9, 2024

this includes cloud resources and k8s. I'll add more tomorrow.

Thanks. Yeah, I'm wanting to know the specific things that we are trying to add here. It isn't clear to me if we are looking to do some existing stuff (env vars, system properties, profiles) differently. For example already have avaje-aws-appconfig for AWS AppConfig. Good to understand clearly where this is heading to, is this looking to replace the existing default loading etc.

@agentgt
Copy link

agentgt commented Oct 16, 2024

Man idk what it is, but this load flags concept feels super complicated and unwieldy. I'm not sure I vibe with it. Do we actually need it?

I just showed how no one needs to know anything about them. They are internal.

I mean you can switch it to:

_sentry_man_does_not_like_required=true

Only the ConfigProcessor know about that special Key Value above and it only allows certain URIConfigLoader to use it. Basically we need or want the URIConfigLoader to communicate back to the ConfigProcessor some special handling. We don't even necessarily need to do the magic _flags if you return something other than Map<String,String>. That is we could do keyValuesLoader instanceof NotRequired r && r.isRequired(). KeyValuesLoader would be the return.

As for Load Flags it is only because it was effective to serialize and represent lots of booleans.

Regardless we need to represent NOT_REQUIRED somehow unless we always have optional load.

Here are our choices some overlap:

  1. We never support required load. I find this a hard pill to swallow.
  2. Each URIConfigLoader decides whether to supports required or not_required loading. It uses some custom URI query flag or variable and then throws whatever random exception. Like I talked about in Discord we end up with the fucking Maven problem where every plugin has some special "skip" property.
    1. We could do some standard abstract class that handles common URI query parameters like ?_not_required=true.
  3. The user somehow marks the URI either in load.properties or _load_something that it is required/not_required
    1. With load.properties it basically would have to be URI query parameter and that parameter would have to be removed for the actual URIConfigLoader. This is actually tricker than you might hink.
    2. We allow other public _load commands like _require and _maybe which is what snaphop-kvs did before load flags. This is important. Overtime I found the need to communicate more and more. How many more _something should I keep adding. Hence Load Flags. However I'm still partial to this version because it is what we do.
  4. The URIConfigLoader communicates that the resources it has in load can be NOT_REQUIRED through some means we find acceptable. It could be some key value that is returned or it could be some special things that we can instanceof on the return URIConfigLoader or it could instanceof on the loader itself. For some reason I'm liking the return key values profiles hack I showed above over instanceof checking.
  5. EDIT forgot about something else I toyed with and is apparently in our URILoadContext is KeyValues kvs = contex.load(uri). Basically the context allows you to load resources. I found lots of problem with this approach though.

@agentgt
Copy link

agentgt commented Oct 16, 2024

Also besides required what if wanted to load a resources with out interpolation.... let it sit in that passwords might contain ${.

I had checked in we had use LoadFlag.NO_INTERPOLATION for that exact reason in some CLI too.

stdin://client_password?_loadFlag=no_interpolation

You see that is why there are so many load flags. Overtime we just kept needing more behavior. This internal library is 13+ years old. The world has changed quite a bit . I had the logic at first in the URI loaders and it was painful. Adding flags was so much easier than the alternatives. All the loaders would get that extra capabilities for free.

@SentryMan
Copy link
Collaborator Author

I'll probably get it when we start implementing

@agentgt
Copy link

agentgt commented Oct 24, 2024

@SentryMan @rbygrave I'm in the process of opensourcing my companies kvs config library.

By doing so I'm ripping out legacy stuff, documenting, and trying to find what is best including ripping out the scary LoadFlags.

I don't plan on publishing it to maven central and would prefer to make avaje-config work but since I have the code I might as well publish it for looking over. It has very little javadoc which I'm fixing in the next few weeks.

My hope is you can pull out the best parts of it you like. However I think what @SentryMan has already done is pretty good. Possibly even good enough with the exception of require/optional lacking. That isn't really that much a problem because one could always make their own URIConfigLoader to forcefully require it (or make it optional).

There are some API changes I think should be made for the safety of long term unchanging API:

I would change:

  /**
   * @param uri uri from which to load data
   * @param parsers map of {@link ConfigParser} available to assist in parsing data, keyed by {@link
   *     ConfigParser#supportedExtensions()}
   * @return key/value map of loaded properties
   */
  Map<String, String> load(URI uri, URILoadContext ctx);
  /**
   * @param uri uri from which to load data
   * @param parsers map of {@link ConfigParser} available to assist in parsing data, keyed by {@link
   *     ConfigParser#supportedExtensions()}
   * @return key/value map of loaded properties
   */
  KeyValueSupplier load(URI uri, URILoadContext ctx);

  interface KeyValueSupplier {
    Iterable<Entry<String,String>> load() throws IOException;
  }

Also instead of

String[] supportedSchemes();

I think

boolean supports(URI uri);

Then you just loop through with the only special handling of file.

This allows the flexibility of doing something like:

myplugin.classpath://blah.properties

Then the supports is:

boolean supports(URI uri) {
  return uri.toString().startsWith("myplugin.");
}

(you can always make an abstract class to do the supportedSchemes() with the above but you can't go the other way).

@rbygrave
Copy link
Contributor

already done is pretty good

This PR isn't close in terms of something we can work off for this issue. It removed the Parsers concept, has changes unrelated to this issue, doesn't keep this change separate enough from existing functionality, plus ultimately I believe the "URI Like" loader distills down to not using the URI type (due to the parameters being provided via the context, and we don't strictly need the prefix, so URI looks to distill down to a path ... but we have modifiers/flags.

So no I don't think this PR provides a good base for this change.

I'll look to create a new issue that outlines what we have distilled this down to.

@agentgt
Copy link

agentgt commented Oct 28, 2024

It removed the Parsers concept,

I'm generally in agreement in terms of it actually being pushed into master or at least the hesitation but I'm unclear on the above as I thought parsers were still in?

I will say that @SentryMan has been so great and put in good/hard work so that might be what is making me feel like it was good enough.

That is if anyone has caused the train to derail a bit I'm entirely to blame.

I'll look to create a new issue that outlines what we have distilled this down to.

Yeah that is a great idea as it is getting confusing with the changes in code and comments. Perhaps this time we wait on implementation and get some sort doc on how it works.

Also I wasn't sure if it is still helpful for me to continue to pseudo open source my companies config library or do you think that might distract @rbygrave ?

I'm still not sold on the URI being not really URI. That was @SentryMan and talked about this format:

_someId_load.properties=profiles.classpath://something.properties
_anotherId_load.properties=...

But we can talk about that on the new issue.

@agentgt
Copy link

agentgt commented Oct 28, 2024

I will say that there several actual bugs that need to be fixed or at the minimum the doc adjusted. I was kind of hoping for those before URI.

#177 #184

@SentryMan which PR is the recursive loading of load.properties? - that one as well particularly given the doc says this:

load.properties is pretty versatile and can even be chained.

It also shows interpolation in the load.properties with the doc but it did not entirely work.

So on one hand this URI stuff might have been a distraction but on the other I think we found some good issues that could be fixed without breaking things I think.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Oct 28, 2024

@SentryMan which PR is the recursive loading of load.properties? - that one as well particularly given the doc says this:

yeah #178 is the one

@agentgt
Copy link

agentgt commented Oct 28, 2024

@SentryMan question did you put in some way to completely disable Avaje Config default loading? I saw a weird boolean somewhere like skipConfig or something.

Because one thought I have so that I can use the Config API is I use our loading mechanism instead of Avaje but I use the Config.getXXX API.

You see I'm big on modularity and our system has the loading entirely separate from the fetching of properties. That is the whole Config API is very similar to this:

https://github.com/agentgt/configfacade

The above was my pseudo opensourcing of our Config API. You'll notice it lacks loading as our system had that separated out. You also notice how it kind of looks like Micro Profile Config.

What is not opensource @rbygrave is the loading of Key Values (or probably Name Value pair is better since there can be duplicates and it is a list with order).

That is in our system we have:

  • mycompany-kvs
  • mycompany-config

Anyway I do realize there is debt to having more modules but it is just a thought if we do massive changes.

@SentryMan
Copy link
Collaborator Author

I added a boolean to disable custom URI loaders, not the built-in ones

@rbygrave rbygrave mentioned this pull request Oct 29, 2024
@SentryMan
Copy link
Collaborator Author

Response to #193

Bootstrap:

I was thinking of having this new mechanism as a compliment to the existing one. That is, what I would like to see is that we initially continue the standard loading process we have now (achieved with the new loaders):

  void loadLocalFiles() {
    loadMain(RESOURCE);
    loadViaProfiles(RESOURCE);
    // external file configuration overrides the resources configuration
    loadMain(FILE);
    // load additional profile RESOURCE(s) if added via loadMain()
    loadViaProfiles(RESOURCE);
    loadViaProfiles(FILE);
    loadViaSystemProperty();
    loadViaIndirection();
    // test configuration (if found) overrides main configuration
    // we should only find these resources when running tests
    if (!loadTest()) {
      loadLocalDev();
    }
    loadViaCommandLineArgs();
  }

and then after that, processing the avaje.load.sources and what have you.

The known flags we want to support are:

Do we actually need them?

specify load order including test
avaje.load.sources=foo bar cmd test aws vault

Should the test loading order not go in the application-test-.properties? Why should this be in the main application.properties?

I was thinking we could load test properties after loading app properties. (like we do now) In this way, a user can override the avaje.load.sources for tests.

@rbygrave
Copy link
Contributor

having this new mechanism as a compliment to the existing one.

It's an option we can look at. We can discuss how we use the new mechanism later once we have a really good idea of how it's going to work [e.g. eval/interpolation impact]. My gut is saying this is can be an alternative that can replace the existing mechanism and that approach will have significant benefits as a "new alternative".

... but yeah, we can go through this again probably when we have a really good feel for the full impact / implications.

we could load test properties after loading app properties. (like we do now)

We need to be careful, in that if we look closely we see that it's ... test aws vault and this is actually the same as how it works today in that aws and vault today are ConfigurationSource and those are loaded last AFTER application-test.properties has been loaded.

(like we do now)

So that's the thing, today test properties are not loaded absolutely last but instead loaded before the ConfigurationSource. If everything is a "New ConfigLoader2 thing" ... then we have test properties not loaded strictly last.

If we search above for "Ordering and test configuration", I discuss the test configuration questions.

Flags ... Do we actually need them?

For MAYBE / NOT_REQUIRED, I think we do need the flag. It might be via the maybe: prefix.
For NOT_INTERPOLATED, we need some way for the stdin source to indicate that the value should skip interpolation.

At this stage, these are the 2 Flags / 2 use cases that we need to support in some way (currently via "Flags").

@SentryMan
Copy link
Collaborator Author

For MAYBE / NOT_REQUIRED, I think we do need the flag.

The how is simple (though I'm partial to ?required=true myself), just want to confirm the reasoning.

For NOT_INTERPOLATED, we need some way for the stdin source to indicate that the value should skip interpolation.

how likely is it that an encoded value passed via stdin would have ${<random stuff>} and also match an existing key such that it would get interpolated? Are there other scenarios where one should skip interpolation?

@agentgt
Copy link

agentgt commented Nov 2, 2024

@rbygrave @SentryMan

I have ported a subset of our configuration framework to OSS.

https://github.com/jstachio/kiwi

I have made it public. It is obviously lacking javadoc and lots of other stuff.

My sole purpose on putting it up is to give you guys ideas and for me to try things.

Besides the readme I guess the unit test kind of shows how it works:
https://github.com/jstachio/kiwi/blob/main/kiwi-kvs/src/test/java/io/jstach/kiwi/kvs/KeyValuesSystemTest.java

There are lots of major differences with avaje config but lots of similarities notably the chaining.

Right now I'm working on the idea that all resources (URI) aka load.properties have a symbolic name.

_load_somename=uri
_flag_somename=flags...

Where somename is the resource identifier and it is [a-zA-Z0-9]+. (the above is what would happen if you always serialized how a KeyValueResource).

Furthermore there is none of this of which I disagree with:

load.properties=uri_maybe uri_sort_of

That is Kiwi pushes hard on the idea of always key values and resources are URI with some name.
This is just what I'm going with currently because invariants often make things simple, easier to comprehend and more robust.

Now here is the shitty part. The library is already 80kb. Avaje in maven central is like 79kb.
I assume I just over model/abstract. However I a single naive unit test hits 75% code coverage so maybe it is just the way it is. This is probably where you guys help on as I have the problem of over engineering.

@rbygrave
Copy link
Contributor

rbygrave commented Nov 4, 2024

Just to say, Kiwi - is a Bird, the National symbol of New Zealand, and a nickname for people from New Zealand.
Kiwifruit - is a the "hairy fruit" and actually a Chinese Gooseberry (it originates from China and not New Zealand but was successfully grown and exported by New Zealand companies as "Kiwifruit").

@agentgt
Copy link

agentgt commented Nov 4, 2024

Yeah the name is a placeholder. In the US Kiwi is fruit from the tree aka kiwifruit. Half our companies business is in APAC and they said the same thing to me but less about the bird and more about NZ (most of our guys are Australian which I think you are as well?).

I picked kiwi cause lol I asked ChatGPT to come up with the closest tree like thing that sounded like key value.

(btw do you think the name is offensive? If so I will change it immediately.)

@rbygrave
Copy link
Contributor

rbygrave commented Nov 5, 2024

Australian which I think you are as well?).

Close but no, I'm a Kiwi :) ... the "Person from New Zealand" variety - not the Fruit :)

do you think the name is offensive?

No, it's not offensive. New Zealand is a small country, we are used to people not knowing much about us etc. It's a standing joke that as a country we are left off most world maps. No worries there.

ChatGPT to come up with the closest tree like thing that sounded like key value

That's pretty funny to me in that Kiwifruit grows on a Vine, I wouldn't actually consider it a Tree per se. ChatGPT was a tad "creative" there.

most of our guys are Australian

Well, if they are into Cricket they will be loving us Kiwis at the moment [with NZ's wins over India in India - truely epic]. Maybe don't talk about the Rugby though.

@SentryMan
Copy link
Collaborator Author

MAYBE / NOT_REQUIRED, I think we do need the flag.

ok but do we actually need this though? For what reason do we need to handle maybe/requires? This flag in particular can be left as an exercise for those who want to implement their own loaders, as Config#get throwing an exception has usually been enough of a signal that some configuration is missing.

@rbygrave
Copy link
Contributor

@agentgt I lived in San Fransisco for a few years and Boston for a few years, so I do realise just how little Americans know about New Zealand. Given you have released "Kiwi" ... I feel obliged to fill you in a little on New Zealand and Kiwi culture.

When you asked if "Kiwi" was offensive I did say that it wasn't offensive and I'd still say that. What I would suggest though, is it "rather uninformed" and being American that is kind of expected per se.

Kiwi are important to us and for example, the Kiwi is on our military uniform:
https://www.1news.co.nz/2023/02/22/king-charles-greeted-with-hongi-by-kiwi-soldier-training-ukrainians/
... here we see New Zealand soldiers with the white Kiwi on the black patch. Although our flag is mostly blue our national colours are actually Black and White, that is our national sporting teams were either Black or White. The Hongi is a Maori greeting of rubbing noses.

The Kiwi is our national bird [like your bald eagle]. New Zealand as a land mass broke off from Gondwana 85 million years ago so we are well known for our unique flora and fauna - the Kiwi is unique to New Zealand and an endangered species [there are only a few places in NZ where you would be able to find and see Kiwi in the wild]. Noting that we have a whole lot of birds and plants that only exist in New Zealand, the "Silver Fern" is a plant only found in NZ and also a national symbol. Kiwis representing New Zealand in sport will either be wearing either the Kiwi or the Silver Fern and typically Black or White uniforms.

Culturally we are a mix of indigenous Maori culture, British culture, Polynesian culture and the usual polygot. Respecting culture is kind of big in New Zealand and we are really big into "Haka" as a Welcome, a Celebration, a sign of Respect, or a Challenge - it has many uses.

If you are important or cool you'll get a Haka as a welcome e.g. Billie Eilesh - https://www.youtube.com/watch?v=ej_lp0ZwckY
Kiwis love our rugby and we perform a Haka befor an international rugby game (e.g. versus England this year) - https://www.youtube.com/watch?v=TXiO6MWCra4

... so yeah Kiwis love a good Haka.

Hopefully some of that is interesting.

Cheers,
Kiwi Rob :)

@agentgt
Copy link

agentgt commented Dec 2, 2024

I'm going to change the name (kiwi).

I'm just trying to decide whether I call it kiwifruit and keep the package/module/maven gav as kiwi as just kind of a abbreviation of "kiwifruit".

Or just give up the tree err plant thing and go with "ezkv" or "kvs" or [insert name].

@rbygrave Thanks for being patient (EDIT as in tactfully slowly explaining the importance of the term while being very kind!).

EDIT I think I will go with "ezkv" as kiwifruit is probably harder of a change as I can't just grep search and replace everywhere. I also won't to try out the maven redirect stuff.

My hesitation of "ezkv" was lack of some symbol for vision memory impression but I think I'm going to go something like

"ezkv" lemon squeezy

And do a lemon and or lemon tree.

What do you think of that name?

@agentgt
Copy link

agentgt commented Dec 2, 2024

Here is currently the new project name and really really crappy AI image.

https://github.com/agentgt/kiwi

I haven't yet merged it back to main and renamed the repo yet.

@agentgt
Copy link

agentgt commented Dec 3, 2024

Project renamed and released into Maven Central:

https://github.com/jstachio/ezkv

I didn't bother doing the maven relocate thing as the library was so new.

I'm really glad @rbygrave was persistent as this would have been more painful to change later.

Also I'm curious what you guys think of my SVG logo. I tried the AI generators and I just could not get DALLE to do what I want so I spent the day manually writing SVG code.

Yes it is cheesy but I hope it makes it just a little bit more memorable.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Dec 3, 2024 via email

@agentgt
Copy link

agentgt commented Dec 3, 2024

Logo's pretty good, kinda wish it had a little more symmetry

Well lucky for you it is just code: https://github.com/jstachio/ezkv/blob/main/etc/ezkv_logo.svg?short_path=cbcb632

So you can try a stab at messing with the SVG. That is one thing I do love about SVG because I struggle more trying to figure out drawing tools than just learning some XML.

I tried to make the keys kind of look like lemon seeds falling out (squeezed). It does at least look like a lemon right? Also @SentryMan I don't have a windows machine so the fonts could be screwed up. I may need to convert those to line paths (basically it draws the font with lines) with macSVG.

If you do can you have a windows machine can you send me a screen shot in discord at some point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom URI based loading of sources extension idea
4 participants