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

feat(api): Ideas/examples for a possible optional JsonType API #691

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cb0s
Copy link
Member

@cb0s cb0s commented Jan 5, 2023

Optional JsonProperties for the Configurations of the Telestion Verticles

Problem

As soon as verticles get complexer, optional JsonProperties for the defining Configuration are helpful to reduce the work by the configurating party - the end user - who expectedly has little to no experience at all with the Telestion Ecosystem. Therefore, these should be deemed a highly important role, making the project a lot more appealing to annoyed software developers, being woken up at 9 am (very early for a developer) to fix the missing parts of the configurations... 😆

Current state

At the moment, there is no special support for this feature apart from the native support out of the box from the Jackson Library or additional libraries. Unfortunately, the Jackson API does not easily support default parameters for inferring other than for documenation reasons as the Java Type System and its stubborness makes a good design difficult (or even impossible) and no possible solution. To be fair, there is one possibility to add those to the existing system. One has to add an additional constructor without one of the fields, which does not only stack up with more and more record parameters (problem 1), but also imposes problems with parameters of the same type in one class (problem 2). An example showcasing both of those problems is seen below. We want to give Default parameters to every parameter:

public record Foo(String param1, int param2, String param3) {
    // No parameters
    public Foo() {
        this("param1Foo", 42, "anotherFoo");
    }
    // Only param 1
    public Foo(String param1) {
        this(param1, 43, "anotherFoo");
    }
    // Only param 3
    public Foo(String param3) {  // <----- actually does not work as this constructor already exists for param 1 -> problem 2
        this("param1Foo", 42, param3);
    }

   // ... -> we need 4 contructors more to cover every edge case (one for param 2 alone and then 3 more for the different pairs)
}

Note that with this solution and the massive amounts of equal implementations also small mistakes as shown in the constructor for String param1 lead to fast bugs in the development of more complex verticles. Here, obviously the 43 should be a 42. This obviously can also be targetted by constant static variables, but imho this is still a bad style of implementation if you need an extra mechanic to fix a problem that shouldn't even exist in the first place.

Implementation Suggestions

In all cases, the manual overhead of adding default information to each of the Record values should be as low as possible. Therefore, an annotation-based approach is suggested which complements the existing @JsonProperty from Jackson. As seen in the examples, the Annotations are only added additionally to the fields. In a future release, it could also be thought of combining those additional Annotations with the JsonProperty into one Property, reducing the overhead even further. In the background a manual ObjectMapper (already provided by the Jackson API) could be used, which could work hand in hand with the proposed new Launcher API by @fussel178 - the registering would take place in this new Launcher API at the start of the Application. Currently, up to this point, the registering alternatively would take place in the Telestion class in the application package.

Details about the implementation

These 4 suggestions show possible implementations for the support of OptionalJsonProperty without relying on a big number of manual constructors as suggested by the creators of Jackson. Consequently, none of those implementations are working at this point and should only be seen as examples of how an API could look like, especially because the rest of the API would be hidden, and only the annotation are used by the users of the Telestion System.

Meta information about this PR

Related links

As far as I know, no current Issue is targetting this problem.

CLA

  • I have signed the individual contributor's license agreement and sent it to the board of the WüSpace e. V. organization.

I would love some feedback and an open discussion about this problem! 👀

These 4 suggestions show possible implementations for the support of OptionalJsonProperty without relying on a big number of manual constructors as suggested by the creators of Jackson. Consequently, none of those implementations are working at this point and should only be seen as examples of how an API could look like, especially because the rest of the API would be hidden, and only the annotation are used by the users of the Telestion System.
@cb0s cb0s added 🌷 enhancement New feature or request 📄 java Pull requests that update Java code 🔧 telestion-api Everything related to the telestion-api module labels Jan 5, 2023
@fussel178
Copy link
Member

fussel178 commented Jan 5, 2023

@pklaschka
Copy link
Member

I believe that the cleanest, most readable, and most flexible approach to go about this is to override the getter methods in the records. Then, it looks something like this:

public record Room(@JsonProperty(defaultValue = "10") int size) implements JsonRecord {
    public Room {
        if (size() < 0) {
            throw new IllegalArgumentException("Size must not be negative");
        }
    }

    public int size() {
        return Objects.requireNonNullElse(size, 10);
    }
}

By "only" having the one constructor (and no need for a default constructor anymore), we can have additional validation in this constructor (as you can see here) which we can also use to determine if a message on the event bus is a valid object of that type (in this case, { size: 20 } would be a valid Room, but { size: -20 } not) in terms of the type parsing, and we can even use more elaborate defaults (e.g., the default port could be different depending on the protocol selected in another parameter).

This also doesn't require developers to learn additional concepts (such as the handling of default constructors or new annotations), can handle complex objects, etc.

The only downside I see with this, which compared to the other methods is in my opinion far less severe in terms of issues, is that you have to be careful to call the actual getter function (instead of the parameter itself) inside the record's functions (e.g., in the validation size() < 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 java Pull requests that update Java code 🌷 enhancement New feature or request 🔧 telestion-api Everything related to the telestion-api module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants