feat: @DefaultValue @UnsetValue and @EmptyValue annotations

[mxtartaglia-sl]

Description: This PR aims to refine our handling of configuration data records. Introduces 3 new annotations:

DefaultValue: This feature simplifies the management of scenarios where explicit values are not provided in the configuration file. Additionally, I've included shorthand versions, EmptyValue and UnsetValue.

UnsetValue: Allows distinguishing between intentionally unset values and those requiring default values.

EmptyValue: Allows distinguishing between blank properties and unset or default values.

Additionally, @ConfigurationProperty is left only to map the name of the property in the file to the field in the record, and the defaultValue property has been marked as deprecated for future removal.

Validations were added to assert that only one version of the annotation is used.

Closes: #12902

[github-actions[bot]]

Node: HAPI Test (Restart) Results

2 tests   2 :white_check_mark:  6m 35s :stopwatch: 2 suites  0 :zzz: 2 files    0 :x:

Results for commit f33b6bc4.

[github-actions[bot]]

Node: HAPI Test (Node Death Reconnect) Results

2 tests   2 :white_check_mark:  7m 4s :stopwatch: 2 suites  0 :zzz: 2 files    0 :x:

Results for commit f33b6bc4.

[github-actions[bot]]

Node: HAPI Test (Token) Results

235 tests   234 :white_check_mark:  20m 53s :stopwatch:  17 suites    1 :zzz:  17 files      0 :x:

Results for commit f33b6bc4.

[github-actions[bot]]

Node: HAPI Test (Crypto) Results

335 tests   335 :white_check_mark:  38m 40s :stopwatch:  25 suites    0 :zzz:  25 files      0 :x:

Results for commit f33b6bc4.

[github-actions[bot]]

Node: HAPI Test (Misc) Results

459 tests   446 :white_check_mark:  40m 4s :stopwatch:  77 suites   10 :zzz:  77 files      3 :x:

For more details on these failures, see this check.

Results for commit f33b6bc4.

[github-actions[bot]]

Node: HAPI Test (Time Consuming) Results

21 tests   21 :white_check_mark:  54m 9s :stopwatch:  3 suites   0 :zzz:  3 files     0 :x:

Results for commit f33b6bc4.

[github-actions[bot]]

Node: Unit Test Results

  2 269 files  + 1    2 269 suites  +1   2h 29m 23s :stopwatch: + 24m 20s 112 343 tests +18  112 276 :white_check_mark: +18  67 :zzz: ±0  0 :x: ±0  120 796 runs  +18  120 729 :white_check_mark: +18  67 :zzz: ±0  0 :x: ±0 

Results for commit f33b6bc4. ± Comparison against base commit ee11e055.

This pull request removes 3980 and adds 3762 tests. Note that renamed tests count towards both.

  
             IssuerDN: CN=s-aaaa
            SubjectDN: CN=s-aaaa
           Final Date: Fri Jan 01 00:00:00 UTC 2100
           Public Key: RSA Public Key [2e:28:bc:1e:d3:83:25:92:8e:cb:98:b1:b6:84:06:9c:d5:d8:14:d5],[56:66:d1:a4]
           Start Date: Sat Jan 01 00:00:00 UTC 2000
         SerialNumber: 12482092706667292405
        modulus: c1a0ff5d2372b53d12d12bb87dd03f5…
   Address[id=0,nickname=Austin,selfName=aaaa,weight=1000,hostnameInternal=127.0.0.1,portInternalIpv4=10718,hostnameExternal=173.100.65.154,portExternalIpv4=45568,sigPublicKey=<null>,agreePublicKey=<null>,sigCert=com.swirlds.platform.crypto.SerializableX509Certificate@62532a51,agreeCert=com.swirlds.platform.crypto.SerializableX509Certificate@5a67e2b6,memo=UGd8C3fDa8],
…

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [4] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [6] 

com.hedera.node.app.grpc.impl.netty.GrpcServiceBuilderTest ‑ [7]   
  
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [10] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@c5f9ebc6
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [11] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@3c3cc36f
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [12] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@fb56f2fa
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [13] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@aac16c99
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [14] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@a764c41c
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [15] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@111d5d7e
com.hedera.node.app.service.mono.state.codec.VirtualKeySerdesAdapterTest ‑ [16] com.hedera.node.app.service.mono.state.codec.VirtualBlobKey@13202cff
…

[github-actions[bot]]

Node: HAPI Test (Smart Contract) Results

588 tests   588 :white_check_mark:  1h 13m 30s :stopwatch:  63 suites    0 :zzz:  63 files      0 :x:

Results for commit f33b6bc4.

[hendrikebbers]

I disagree with that change. This will make the API way more complex. Today, you only need to know the ConfigProperty annotation. That one is well documented in JavaDoc, ConfigData annotation and the Config API documentation.

If you have a really long definition it will look like that:

@ConfigProperty(value = "thisIsTheName", defaultValue="thisIsTheDefaultValue") String foo

In mostly all our use cases we do not specify a custom property name (but use the field name). In that case the property definition looks like this:

@ConfigProperty(defaultValue="thisIsTheDefaultValue") String foo

The only benefit of this PR is that you can now write:

@ DefaultValue("thisIsTheDefaultValue") String foo

While this is shorter (since the term DefaultValue has fewer chars than the term ConfigProperty and you do not need to write defaultValue=), it introduces a new annotation. That one needs to be documented and understood by a developer.

While you can easily argue that it is just one simple additional annotation that a dev must learn we will come to that point again. It is wrong from my point of view to externalise annotation properties in "standalone" annotations. Let's take the JPA spec as an example. You define a database column in JPA like this:

@Column(name="DESC", nullable=false, length=512)
private String description; 

Here, the name is also optional. However, JPA does not provide a Nullable or Length annotation. Why? Let's assume what could happen in a bad case:

@Column(name="DESC", nullable=false, length=512) @Nullable(true) @Lenght(128)
private String description; 

Now we have a problem: What nullable and length value should be used? Sure, you can now say that only one should be allowed, but that again creates additional complexity.

At the end my thought is like that: each annotation / interface / class adds additional complexity and we need to find a good comprise between readability, complexity, maintainability, ... This special case is a use case where I see the compromise in not adding an additional annotation

[mxtartaglia-sl]

@hendrikebbers I agree with the complexity part of your argument, but only in the sense that we need to extend the framework to support these annotations with complex code. We need to take care that no multiple annotations are used, and communicate the adoption. But guess that to assume the complexity to provide users with flexibility is also the goal of a framework. As counter examples of having compact annotations javax.validation framework or even spring mvc providing syntax sugared versions for @RequestMapping and the endpoint definitions.

The intention of the PR is to separate concerns and to add flexibility and overcome some limitations that i think comes from jampacking properties in the same annotation: Here are some examples:

 public record ConfigRecord(@ConfigProperty String property){} //possible and redundant
 public record ConfigRecord(@ConfigProperty(defaultValue = UNDEFINED_VALUE) String property){} //possible but redundant as it achieves nothing and is the same as not including the annotation
 public record ConfigRecord(@ConfigProperty(defaultValue=NULL_DEFAULT_VALUE) String property){} //assigns null
 public record ConfigRecord(@ConfigProperty(defaultValue=NULL_DEFAULT_VALUE) int property){} // breaks
 public record ConfigRecord(@ConfigProperty(defaultValue="This should be only one value, but will be two") List<String> property){} // breaks the value because there is a comma
 public record ConfigRecord(@ConfigProperty(defaultValue="") String property){} // maps to a string of size 0 
 public record ConfigRecord(@ConfigProperty(defaultValue="[]") String property){} // maps to a string of size two 
 public record ConfigRecord(@ConfigProperty(defaultValue="") List<String> property){} // maps to a list of size 0 
 public record ConfigRecord(@ConfigProperty(defaultValue="[]") List<String> property){} // maps to a list of size 0 

With the new schema you have different annotations supporting different scenarios:

if i want to map a Record field to a mandatory property on the file:

   public record ConfigRecord(String property){}

if the property on the file has an invalid name for java:

 public record ConfigRecord(@ConfigProperty("class") String className){}

if i want to map same config property to different fields

 public record ConfigRecord( int number, @ConfigProperty("number") String numberAsString){}

And allows to be clear from when somebody is doing something requiring Default values, if i want to set a default value in a field making the existence of the property not mandatory in the config file:

 public record ConfigRecord(@DefaultValue("value") String property){}

if i want the existence of the property not mandatory in the config file, but still not interested on providing a default value and want java initialize the field:

 public record ConfigRecord( @UnsetValue String property){}

if i want the value to be initialized to Empty:

 public record ConfigRecord( @EmptyValue String property){}

I would go as far as saying that this version of the pr is not enough, and that the 3 annotations should be handled separately given that the each one of them have different syntactical meaning.

 public record ConfigRecord(@DefaultValue({}) String property, @DefaultValue({}) int propertyInt, @DefaultValue({}) char propertyChar){}. //should fail in each case
 public record ConfigRecord(@DefaultValue("") String property){}. //should map to empty string 
 public record ConfigRecord(@DefaultValue({}) List<String> property){} // should map to empty list or set
public record ConfigRecord(@DefaultValue("") List<String> property){} // should map to a list with only one element being ""
public record ConfigRecord(@DefaultValue("A,B,C") List<String> property){} // should map to a list with only one element being "A,B,C"
public record ConfigRecord(@DefaultValue({"A","B","C"}) List<String> property){} // should map to a list with 3 elements being "A", "B" and "C"

In that case the @EmptyValue annotation wouldn't be necessary, you just need to understand that the empty initializations are different for string and for collections.

IMO jampack different syntax meaning into one annotation creates unnecessary thought process remapping when using the framework, that is more expensive than remembering to use a one different annotation for a separated use case.

[hendrikebbers]

As counter examples of having compact annotations javax.validation framework or even spring mvc providing syntax sugared versions for @RequestMapping and the endpoint definitions.

javax.validation is not a counter-example. The goal is to have an API that can be extended by custom annotations. Since any annotation can be added for validation, you can not have 1 big annotation. Next to that, javax.validation contains annotations that only make sense for some specific data types (min/max). Based on those points, it makes sense to have individual annotations. For the config API, we have the same validation. The idea is to add custom annotations if you need specific validations. The API allows that similar to javax.validation. For the config property definition, you do not need to extend anything. Based on that, it makes sense to have 1 annotation for that.

@RequestMapping in spring does something that goes in a direction that is more similar to the point that we discuss here. The annotation is used to define an REST endpoint, which has a lot of parameters. Since you always need to set the HTTP-Method-Type that is based on an enum, you have specialized annotations for each method Type (like @GetRequest) that act exactly like @RequestMapping but have 1 value already predefined. While that is quite easy in the first place, you will find several questions and documentation about it online (https://medium.com/@alsigitguntoro/what-a-diff-requestmapping-and-getmapping-annotation-9de4ba41694f#:~:text=In%20summary%2C%20'%40RequestMapping',when%20dealing%20with%20GET%20request.). That underlines my point that such API extension needs way more documentation and knowledge. Next to that our defaultValue is not an enum. This PR already contains 3 new annotations to set defaultValue to specific types. Especially UnsetValue and EmptyValue are way more complex than the annotation overloading that spring does. Without really thinking about it I could not tell you what that does for int, List<String> or Date, String. Since defaultValue is no enum, what comes next? @DefaultIs17, @DefaultIsFalse (would be shorter than @DefaultValue(false)) - just kidding ;) Based on that I still see it different to the spring sample.

[hendrikebbers]

Regarding Spring: Let's assume you have the following method:

@GetMapping("/get1")
@RequestMapping(value = "/get2", method = {RequestMethod.GET})
public String getData() {
    return "Foo";
}

Any idea what REST endpoint this code creates? /get1, /get1, or both? JavaDoc does not give a clue here. We would have the same with:

 public record ConfigRecord(@EmptyValue @ConfigProperty String property){}

or

 public record ConfigRecord(@DefaultValue("foo") @ConfigProperty(defaultValue="bar") String property){}

[hendrikebbers]

Maybe we should split the discussion of DefaultValue and EmptyValue+UnsetValue.

[hendrikebbers]

Oh, and I know I initially agreed with @cody-littley once he asked me about this change. But the more I think about it the more I do not see the readability benefit against more complexity.

[hendrikebbers]

I converted this PR to draft since the base team agreed on rethinking how we want to define such APIs in general.

[mxtartaglia-sl]

Some opinions:

• Providing a shorthand version of @ConfigProperty(defaultValue="X") will be useful

• making defaultValue property in @ConfigProperty an alias for value would create scenarios were a) readability decreases: e.g: @ConfigProperty("false") b) the use is confusing e.g: @ConfigProperty("someProperty"), does it mean that "someProperty" is the value or is the property name?

• We should move away from the use of these constants of ConfigProperty, as they provide a hack to deal with specific use cases but they are neither safe nor clear to use. Also in case we add more than one annotation, they need to be supported and referenced by other annotation too:

    /**
     * A constant that is used to check if a default is defined.
     */
    String UNDEFINED_DEFAULT_VALUE = "com.swirlds.config.value.unconfiguredValue#1j8!-235u-hBHJ-#nxs-!n2n";

    /**
     * A constant that is used to check if a default is defined as null.
     */
    String NULL_DEFAULT_VALUE = "com.swirlds.config.value.nullValue#1j8!-235u-hBHJ-#nxs-!n2n";
   

• In terms of numbers I agree, 2 seems ok, more is a lot. ConfigProperty, UnsetValue, EmptyValue and DefaultValue are more annotations than we need, but we need to differentiate at least the user provided default value vs the "i do not care if the property is found in the config files" use case. Maybe @DefaultValue(mandatory=false) is a good replacement for UNDEFINED_DEFAULT_VALUE.
• Parsing values for list is not optimal: @ConfigProperties(defaultValue="A,B,C") should be consistent if used in Strings or List or sets. They should map to a single String "A,B,C" or we are informally not supporting "," character as a value only if the type of the property being mapped is a collection.
• Complementary annotations are not a big problem to have, for me is more clear and dynamic. I would not support defaultValue both as ConfigProperty's property and as a separate annotation long term. I see no drawback on using

      public record SomePropertyMapping(@ConfigProperty("aName") @DefaultValue("AValue") String propA,
            @ConfigProperty("aName2") @DefaultValue(mandatory=false) String propA,
            @DefaultValue("AValue") String aName3,
            @DefaultValue(mandatory=false) String aName4,
            String aName5){}  
  

  But i am willing to compromise on this given that we can control the scenarios were both defautValue and @DefaultValue are used in the same time in the annotation processor, and this is fairly similar to the above version:

    public record SomePropertyMapping(@ConfigProperty(name="aName" , defaultValue ="AValue") String propA,
          @ConfigProperty(name="aName2", mandatory =false) String propA,
          @DefaultValue("AValue") String aName3,
          @DefaultValue(mandatory=false) String aName4,
          String aName5){}  
  

[jasperpotts]

What about a middle ground. I see @hendrikebbers argument for not having too many annotations. What about just reducing the name lengths from: @ConfigProperty(defaultValue="X") to @Config(default="X")

[edward-swirldslabs]

Is this PR still relevant?