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

Extracting property source. #139

Closed
agentgt opened this issue Apr 29, 2024 · 6 comments · Fixed by #140
Closed

Extracting property source. #139

agentgt opened this issue Apr 29, 2024 · 6 comments · Fixed by #140
Assignees
Milestone

Comments

@agentgt
Copy link

agentgt commented Apr 29, 2024

While working on Rainbow Gum's generic agnostic property mapping validation and error message I noticed there is no way to get a CoreEntry (or analog).

That is I need the CoreEntry to get the source so that I can build better error messages.

The only way now to get the source information (e.g. like line number) is to use getAs which runs your conversion code as a Function.

Unfortunately I ended up designing Rainbow Gum where that is not really desirable as it creates very confusing call stacks.

What it instead would be easier to some wrapper object like the concrete CoreEntry so that you can do something like:

Optional<PropertyEntry> getOptionalEntry(String key). (and the non optional equivalent if you like). You can bike shed the name PropertyEntry but hopefully you get the idea.

In lightbend aka typesafe config the above analog is called ConfigValue: https://lightbend.github.io/config/latest/api/com/typesafe/config/ConfigValue.html

@rob-bygrave
Copy link
Contributor

Hi @agentgt, if you get a moment can you check that PR to check it satisfies the requirement?

Note that currently in the PR theOptional<Configuration.Entry> configuration.entry(key) method is on Configuration and I haven't added it to Config yet as I don't think it will be "frequent use" per se. So people needing it via Config can access it via Config.asConfiguration().entry(key) if they need it which I think is ok.

Cheers, Rob.

@xabolcs
Copy link

xabolcs commented Apr 30, 2024

... can you check that PR to ...

For the record: that PR is the #140

@agentgt
Copy link
Author

agentgt commented Apr 30, 2024

@rob-bygrave I believe so. What I will do is pull the PR and try it against rainbowgum.

As minor critique of the PR I would like to see a unit test access Configuration.Entry#source() to confirm the goods are there :)

@rbygrave
Copy link
Contributor

rbygrave commented Apr 30, 2024

see a unit test access Configuration.Entry#source() to confirm the goods are there :)

Ha ha, nice catch. There was such a test but I changed entry() to filter out entries that represent null and yeah looks like I managed to remove the only assert on that when I made that change.

@agentgt
Copy link
Author

agentgt commented Apr 30, 2024

@rbygrave it appears to work albeit it does not have line number for file based properties like typesafe config.

I'm OK with that not being present given avaje-config is for simple properties and that would probably just add bloat.

If you don't mind ping me if you guys release this so I can merge the experimental rainbowgum branch into main.

Cheers!

@agentgt
Copy link
Author

agentgt commented Apr 30, 2024

For fun here is an example of rainbowgum working with config PR:

application-test.properties:

logging.level.my=TRACE
logging.global.change=true
logging.change.my=true
logging.appenders=stuff
logging.appender.stuff.output=blah # <- this is bad

The error messaging is a work in progress but you see here it cannot convert the property
to an output (like a logback appender).

io.jstach.rainbowgum.LogProperty$PropertyConvertException: Error for property. key: 'logging.appender.stuff.output' from AVAJE(resource:application-test.properties)[logging.appender.stuff.output], java.lang.RuntimeException Output type for URI: 'blah:///' not found.
Tried: 'logging.appender.stuff.output' from AVAJE(resource:application-test.properties)[logging.appender.stuff.output]
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogProperty$Result$Error.value(LogProperty.java:482)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultAppenderRegistry.appender(LogAppenderRegistry.java:150)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultAppenderRegistry.appender(LogAppenderRegistry.java:87)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultAppenderRegistry.defaultAppenders(LogAppenderRegistry.java:62)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogRouter$Router$Builder.build(LogRouter.java:371)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogRouter$Router$Builder.build(LogRouter.java:311)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.RainbowGum$Builder.build(RainbowGum.java:268)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.RainbowGum$Builder.build(RainbowGum.java:249)
	at io.jstach.rainbowgum.avaje/io.jstach.rainbowgum.avaje.AvajePropertiesProviderTest.lambda$0(AvajePropertiesProviderTest.java:19)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.RainbowGumHolder.get(RainbowGum.java:369)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.RainbowGum.of(RainbowGum.java:80)
	at io.jstach.rainbowgum.avaje/io.jstach.rainbowgum.avaje.AvajePropertiesProviderTest.test(AvajePropertiesProviderTest.java:20)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: Output type for URI: 'blah:///' not found.
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultOutputRegistry.provide(LogOutputRegistry.java:132)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogOutput.lambda$0(LogOutput.java:95)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultAppenderRegistry.lambda$7(LogAppenderRegistry.java:189)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogProperty$PropertyFunction.apply(LogProperty.java:55)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogProperty$PropertyGetter.lambda$0(LogProperty.java:1041)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.ResultFuncGetter.get(LogProperty.java:1233)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.LogProperty$PropertyGetter.get(LogProperty.java:665)
	at io.jstach.rainbowgum/io.jstach.rainbowgum.DefaultProperty.get(LogProperty.java:1102)
	... 14 more

@rbygrave rbygrave self-assigned this May 1, 2024
@rbygrave rbygrave added this to the 3.14 milestone May 1, 2024
rbygrave added a commit that referenced this issue May 1, 2024
* #139 Add Configuration.Entry with info on the source of an entry

* #139 Add missing test asserts for Configuration.Entry source

* #139 Add test assert for Configuration.Entry source of "SystemProperty"

---------

Co-authored-by: Rob Bygrave <robin.bygrave@gmail.com>
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 a pull request may close this issue.

4 participants