Some considerations on Beans...

[gamerover98]

Foreword

These days, I spend a lot of time testing the behavior of the BeanProperty.
As I read in the wiki:

ConfigMe will never return a bean in which a field is null. If it does not succeed to set a field's value, the mapping is considered to have failed and the default object is used. It is therefore important to initialize fields as shown with the country name above if you don't want the absence of one field to make ConfigMe ignore all fields and use the default bean object provided on initialization.

And

As with all other properties, bean properties never resolve to null, i.e. when you do settingsManager.getProperty(property) it is guaranteed to never return null.

But I think I broke these rules in a simple way, let me explain:
I have two bean classes called A and B, where B is used twice in A like the following code:

@Getter @Setter // lombok
public class A {
    private B b1;
    private B b2;
}
...
@Getter @Setter // lombok
public class B {
    private boolean enabled;
    private String value;
}

I have defined a SettingsHolder class that has only one property:

public class ConfigHolder implements SettingsHolder {
    private static final A DEF_VALUE;

    static {
        B b1 = new B();
        b1.setEnabled(true);
        b1.setValue("");

        B b2 = new B();
        b2.setEnabled(true);
        b2.setValue("");

        DEF_VALUE = new A();
        DEF_VALUE.setB1(b1);
        DEF_VALUE.setB2(b2);
    }

    public static final BeanProperty<A> TEST_PROP =
            newBeanProperty(A.class, "a", DEF_VALUE);
}

As you can see, the DEF_VALUE has no null references.

The tests

So, I have created the following unit test to put under pressure the BeanProperty.

@Test
void test() {
    // first file load
    File yamlFile = new File(tempDir.toFile(), "test.yml");
    SettingsManager settingsManager =
            SettingsManagerBuilder
                    .withYamlFile(yamlFile)
                    .configurationData(ConfigHolder.class)
                    .useDefaultMigrationService()
                    .create();

    B b1 = new B();
    b1.setEnabled(true);
    b1.setValue("test1");

    // I don't set B2, so the Bean will be serialized without B2.
    A a = new A();
    a.setB1(b1);

    // At this time, ConfigMe should not save the Bean that has
    // a null reference, but ok it is serialized without that null field.
    settingsManager.setProperty(ConfigHolder.TEST_PROP, a);
    settingsManager.save();

    // Read A from the current settings manager
    a = settingsManager.getProperty(ConfigHolder.TEST_PROP);

    Assertions.assertNotNull(a); // Ok
    Assertions.assertNotNull(a.getB1()); // Ok
    Assertions.assertEquals("test1", a.getB1().getValue()); // Ok
    Assertions.assertNull(a.getB2()); // Ok as I expected.

    // Now try to reload the configuration by recreating the new settings manager instance.
    settingsManager =
            SettingsManagerBuilder
                    .withYamlFile(yamlFile)
                    .configurationData(ConfigHolder.class)
                    .useDefaultMigrationService()
                    .create();

    // Read again A but this time from the new settings manager instance.
    a = settingsManager.getProperty(ConfigHolder.TEST_PROP);

    // What is expected: The reader won't recognize 
    // the Bean (A), and it will set the default value.
    Assertions.assertNotNull(a); // Ok
    Assertions.assertNotNull(a.getB1()); // Ok
    Assertions.assertEquals("", a.getB1().getValue()); // Ok
    Assertions.assertNotNull(a.getB2()); // Ok
    Assertions.assertEquals("", a.getB2().getValue()); // Ok
} 

Considerations

So, did I lose all the data just because of a missing field? Is that correct?
Imagine a user who misses a simple detail while setting up the bean. Will he lose all the data due to that mistake? Perhaps this should be revisited.

Thanks a lot ljacqu 💯

[ljacqu]

Yes, this is currently all expected. The philosophy of ConfigMe is not to confront developers with null by avoiding it. But as you see, ConfigMe puts some trust in the developer: it assumes that the default value doesn't have any null values (but never checks), and it assumes that any object set to a bean property doesn't have any null values either.

That's the theory—your point is valid that a simple missing field might invalidate all data in a config file, which can happen by inadvertence as in your example, or for example by a migration with bugs. One way to mitigate this is to initialize all fields with a value. If B.value had a default value, and if in A your fields were B b1 = new B(); and B b2 = new B();, then ConfigMe can fall back to default values whenever it needs to.

This is a bit cumbersome to do, especially having to initialize every string field to an empty string to have this behavior. One idea could be to allow to define custom default values (on MapperLeafType? somehow configurable?). As an aside, I've been thinking that having more annotation support for fields would be nice, e.g. to define a custom property type just for one field instead of needing to register it with the bean mapper.

Another consideration to keep in mind is that ConfigMe will return whether the configuration is fully valid (ConfigurationData#areAllValuesValidInResource); if a field is missing I would still want ConfigMe to notice it even if it recovers, so that a resave of the config with all fields can be triggered when desired.

Do you have any thoughts on how the bean mapper could be improved? I'm currently reworking how it works so this is the ideal time 😄

[ljacqu]

This is a somewhat similar issue as in #135, except that #135 additionally has the requirement to support all-arg constructors. So whatever mechanism we have for default values, we need to keep all arg constructors and records in mind. I'm currently working on supporting Java records, though I think for any advanced behavior (like different export name, default values) the answer will very quickly just be "use a bean class".

Records are currently kicking my ass so I haven't looked much into supporting all-arg constructors, but the rough draft in my mind is to allow something like this:

public class Country {

  private final String name;
  private final int pop;

  @BeanCreator
  public Country(@Param("name") String name,
                 @Param("pop") int pop) {
    this.name = name;
    this.pop = pop;
  }
}

Heavily influenced by Jackson's @JsonCreator and @JsonProperty. There is no good way to get parameter names in Java unless classes are compiled with -parameters and even then it seems to be Java 8+. I don't want to go down that road.
Unfortunately, from what I've tested, Kotlin data classes do keep param names, but it's not possible to get them from Java. Ugh. So right now I'm ignoring Kotlin data classes, and I'm thinking my best bet is to make it easy to provide custom ways of creating beans for whoever does think they can get it working.
But, eyeing the @BeanCreator example, I do wonder what the best way would be to have default values there. If a value for a @Param is null, check the field and use its default value if it exists? I want to allow different ways of creating classes since it comes up time and time again, but the difficulty lies in keeping it intuitive and consistent across different ways to create it. Just because a developer changes from zero-args constructors to an all-args one, it shouldn't change the way other things are defined :/

Edit: Need to handle @ExportName on records because annotations targeting fields can be added to records.