No Exception for missing fields in Mapped Objects
brainbytes42 opened this issue · 16 comments
Hi,
related to #139, I ran into an issue, that the parser works, when it shouldn't... :-D
My already known TOML-Example ...
[main]
# bla = 1
[parts]
[parts.partA]
hello = "World"
foo = "Bar"
# bla = 42
[parts.partB]
hello = "Gestalt"
extends = "partA"
... gets mapped to a java record defined as record(MyMain main, Map<String, MyPart> parts).
If 'main.bla' is missing in the configuration, but is read directly via gestalt.getConfig("main.bla", int.class) or as a (primitive) field inside another record record MyMain(int bla), I'll get an Exception, which is correct (' MISSING_VALUE, message: Unable to find node matching path').
BUT: If I have a primitive (non-nullable) in my Parts, e.g. record MyPart(String hello, String foo, int bla) and this gets read, I'll receive for 'parts' a Map<String, MyPart> having a key 'partA', but a value assigned of null. (Same for partB, as it misses bla as well.) So I receive an invalid map, but no error message in this case...
Hi,
Can you please try some of the configuration in https://github.com/gestalt-config/gestalt?tab=readme-ov-file#gestalt-configuration. Specifically treatNullValuesInClassAsErrors and treatMissingValuesAsErrors.
Gestalt is a little forgiving by default and will in some places allow null values.
You can set the configuration in the builder like
GestaltBuilder builder = new GestaltBuilder();
Gestalt gestalt = builder
.addSource(ClassPathConfigSourceBuilder.builder().setResource("default.properties").build())
.addSource(ClassPathConfigSourceBuilder.builder().setResource("dev.properties").build())
.addSource(MapConfigSourceBuilder.builder().setCustomConfig(configs).build())
.useCacheDecorator(false)
.setTreatNullValuesInClassAsErrors(false)
.build();Let me know if you cant get the expected behavior with these config flags.
Hi,
thanks for the hint.
I'v tried some of the settings:
- setTreatNullValuesInClassAsErrors
- does not change the behaviour, I still get a Map with null-values
- setTreatMissingArrayIndexAsError
- sounds to me exactly like what I would need, but for Lists, not for Maps. Does not change the behaviour.
- setTreatMissingValuesAsErrors
- Gives me an Exception as Expected.
- setTreatWarningsAsErrors
- Gives me an Exception as well, which contains "WARN, message: Map key was null on path".
So I'll use the third one, as it makes sense to me.
Nonetheless, it was quite confusing for me, that a missing value for an object, which is directly referenced as path, throws an error, and for the same object, but referenced as map, it fails silently and just gives a null-value for the map... Put the other way round - souldn't the directly referenced object be null as well, but not throw an error?
Edit: I just noticed, both settings useful for my Map-Problem also make the usage of Optional impossible, as instead of returning optional.empty, if there is no matching path, an exception is thrown... uh.
Thanks for the update. I will have to make a consistency pass on some of these. They were all written at different times so may not all have the same behavior they should. Then throw into it that Toml and Hocon have a different behavior than properties and json.
For my EnvVars, I am using Optionals again, as not all are mandatory... With this, I ran into this issue again - it's only possible to have an Exception thrown if values are missing OR to have an empty Optional allowed...
Maybe a first solution would be to not classify a missing value for an Optional-field as warning? (Because obviously, if declared optional, the field isn't expected to be set in every case.) Then it would still be possible to fail on missing values and handle warnings as errors (to be safe for the other parts of the config), but Optional values would be allowed to be missing. What do you think? :-)
(I tried setting @config with defaultVal="", but still there will be an exception 'MISSING_VALUE, message: Unable to find node matching path' if I have fail on missing values and handle warnings as errors set.)
Edit:
Some debugging-session later...
- For the containing "root" record, a check for Optional is performed - but the record has it's own class ant is no Optional.
gestalt/gestalt-core/src/main/java/org/github/gestalt/config/GestaltCore.java
Lines 439 to 451 in 42f9d53
- Then, coming from the RecordDecoder, the next node is searched, but not found. But the typeCapture recognizes the filed to be an Optional... So this should mean to not add the errors, but to handle the missing value as Optional.empty like in the Optional-check mentioned before...
- ... which is actually done a few lines later
Where the value gets decoded as Optional.empty, but still says, there was an error. - Lateron, there is a check if some errrors should fail ...
gestalt/gestalt-core/src/main/java/org/github/gestalt/config/GestaltCore.java
Lines 416 to 422 in 42f9d53
... but unfortunately, there seems to be no more link between the result's errors-List and the (correctly decoded Optional-values, which are in the result as well...
Hope this helps... :-)
This information helps a lot, and i have a fairly good idea of what is going on. But if possible can you provide up a unit test, with the expected results. So i can try and run the example and see the same traces and results.
I do agree on your reasoning, that optionals could be treated as acceptable to be missing. Since by definition Optional are optional and a missing one is not an error or an exception. However, i would want to make it configurable, as not everyone may have the same opinion.
The issue is in:
We get the result then return it. So if it is missing the we will get an error for missing value.
I can add something here to convert the missing error to a missing optional error. I would still like to capture that it is missing, but if it is a missing optional error i can manage it separately.
I've put together a test to simulate those cases. Maybe to sum it up, I'm looking for a setting to be sure, every expected value is available and not to miss any mis-configurations (eg. null-value in map, because of missing value), but on the same time, it should be possible to explicitly allow for missing values or defaults without providing another layer of mapConfiguration (which is my current workaround).
import org.github.gestalt.config.Gestalt;
import org.github.gestalt.config.annotations.Config;
import org.github.gestalt.config.builder.GestaltBuilder;
import org.github.gestalt.config.exceptions.GestaltException;
import org.github.gestalt.config.source.MapConfigSourceBuilder;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
class ValidMissingValuesTest {
public record MandatoryValue(String someValue, String mandatory) {}
public record ContainsMandatoryMap(String someValue, Map<String, MandatoryValue> mandatoryMap){}
public record OptionalValue(String someValue, Optional<String> optional){}
public record WithDefault(String someValue, @Config(defaultVal = "myDefault") String withDefault){}
private Gestalt gestalt;
@BeforeEach
public void beforeEach() throws GestaltException {
Map<String, String> configs = new HashMap<>();
configs.put("someValue", "someRandomValue");
// "mandatory" is missing by intention
configs.put("mandatoryMap.foo.someValue", "someRandomValue");
gestalt = new GestaltBuilder()
.addSource(MapConfigSourceBuilder.builder().setCustomConfig(configs).build())
/*
If none of those options is set, the second test fails, but all others are successful;
if at least one is set, the first two tests are successful, but the last two are failing.
*/
.setTreatMissingValuesAsErrors(true)
.setTreatWarningsAsErrors(true)
.build();
gestalt.loadConfigs();
}
@Test
public void FailIfMandatoryIsMissing() throws GestaltException {
Assertions.assertThrows(
GestaltException.class,
() -> gestalt.getConfig("", MandatoryValue.class),
"Missing mandatory value should throw."
);
}
@Test
public void FailIfMandatoryIsMissingInMap() throws GestaltException {
// by default returns null-map-value 'ContainsMandatoryMap[someValue=someRandomValue, mandatoryMap={foo=null}]'
Assertions.assertThrows(
GestaltException.class,
() -> gestalt.getConfig("", ContainsMandatoryMap.class),
"Missing mandatory value should throw instead of resulting in null-map-value."
);
}
@Test
public void AllowOptionalForMissingKeys() throws GestaltException {
OptionalValue optionalValue = gestalt.getConfig("", OptionalValue.class);
Assertions.assertTrue(optionalValue.optional.isEmpty(), "Optional should be a valid default and not throw");
}
@Test
public void AllowDefaultForMissingKeys() throws GestaltException {
WithDefault withDefault = gestalt.getConfig("", WithDefault.class);
Assertions.assertEquals("myDefault", withDefault.withDefault(), "Default value should be valid for missing key and not throw");
}
}Thanks for the great feedback and unit tests to validate changes for your use case.
I am looking into this issue, it may take a while longer as it impacts several parts of the library. Error handling is probably one of the most complex parts of the library as i try and provide multiple errors for each call instead of throwing an exception right away.
I am actively looking into this issue as i have time.
I'm happy to help - and thank you for all your efforts and great work!
I'll be two weeks off, so never mind. ;-)
Please try https://github.com/gestalt-config/gestalt/releases/tag/v0.25.0 and let me know if it works for you.
From my first impressions: Well done, thank you! :-)
Thanks for reporting all these issues, you have kept me busy over the last few weeks. But it also helped improve Gestalt. The complex decoders should be a lot more consistent and easier to understand errors.
I know - but as much effort it is from time to time - it's making good things even better! 🙂
One question remains, though: what is the meaning of 'Gestalt'? I know the German word, but I doubt it is the meaning intended?
In English it means an organized whole that is perceived as more than the sum of its parts. as a configuration library you don't do much but you are organizing a whole project to make it a sum of more than its parts.
I was also thinking of naming it keystone, but that was a but too presumptuous.