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

[REQ] [Java/Spring] Adds Accessors to Models #20453

Open
slobodator opened this issue Jan 13, 2025 · 2 comments
Open

[REQ] [Java/Spring] Adds Accessors to Models #20453

slobodator opened this issue Jan 13, 2025 · 2 comments

Comments

@slobodator
Copy link
Contributor

slobodator commented Jan 13, 2025

Is your feature request related to a problem? Please describe.

Models (requests and responses) can have fields assigned to nulls. Handling them may cause famous Null Pointer Exception.

Describe the solution you'd like

I am about to add a new configuration boolean parameter modelAccessors.

Value Beaheviour
false (default) do nothing, keep current behaviour
true adds accessors according to the field settings
Fields Settings (private) Field (public) Getter (public) Accessor Comment
required:true Foo foo; Foo getFoo() Foo foo()
required:false, nullable:false Foo @Nullable foo; Foo @Nullable getFoo() Optional<Foo> foo() Getter and accessor are different only at this case
required:false, nullable:true JsonNullable<Foo> foo; JsonNullable<Foo> getFoo() JsonNullable<Foo> foo()

Once using that accessor instead of the getter will force the developer to property handle Optional types if needed.

Describe alternatives you've considered

There is an awesome @MelleD useOptional:true with a minor confusing issue and its further discussion. It you're not confused with Optional getters it definitely fits you.

Additional context

Importunely, there is no single approach of using Optional. First, I would suggest getting familiar with various camps https://nipafx.dev/inside-java-newscast-19/

I seem to be from the #2½ one with some extra thoughts. Let me explain my way of thinking.

Statement 1. I guess that fields within a class should be of raw types not Optional. Even I haven't encountered any issues with serialization yet, it is not recommended by Java fathers and static analyzers.

Statement 2. Thus, setters and getters should be of the raw types too. Just to follow the rules of Java Beans specification and not to confuse any framework. There should no extra logic within them, they should match their fields and types etc. Even Jackson Object could handle Optional and MapStruct is forced to do the same I don't think it is fine and correct.

Statement 3. I believe that using setters and getters at the business code is the second billion dollars mistake after NPE. The reasons for that are:

  • setters allow building an object in inconsistent state (a constructor should be used instead) and break the object encapsulation and its business constraints
  • getters make the object "naked" indirectly violating its encapsulation
    Thus, getters and setters (and empty constructors) should be provided for frameworks and not consumed by developers.

Unfortunately, there is no way to restrict that, and the worldwide habit is to use them making an object a structure managed in the procedural programming style

Statement 4. Despite the statement above, requests and responses are DTOs not encapsulated objects. Thus, it is ok and necessary to have direct access to their internals. As @Nullable annotation at the getter is just a warning and changing the getter signature is not allowed according to the Java Bean specification we need the 3rd way -- by adding the accessor. It will be of either a raw type or Optional one based on if the field is required or not as written at the table above.

If the field is non-required its implementation will be

public Optional<Foo> foo() {
   return Optional.ofNullable(this.foo);
}

To avoid any theoretical discussion, let's focus on its practical usage.
Once one is tired with handling NPEs in Java6 way

if (request.getFoo() != null) {
   // do something

... there will be 2 options for them

  • to use useOptional:true. It will lead to changing getters to `Optinal getFoo(), the project will be stopped being complied and this is actually fine as it forces the developer to review all usages of getters.
  • to use this feature of accessors. Still, if their colleagues are against the idea and keep using getters there will be no benefit.

The key point of the accessors is:

  • there are situations where one needs just to bypass the value. Assume, I am storing a JPA entity. JPA doesn't support Optional for fields out of the box AFAIK. Then I'm using the getter
repo.save(
  new Entity(
    request.getFoo() // or request.foo().orElse(null)
  )
);
  • there is some business logic based on the value. That the accessor should be used
if (request.foo().length() < 2) { // is not compiling, telling me that `foo` is optional, forcing to handle it properly
  ...
}

@MelleD There main question is to you as you're from the tech committee. Will you accept this PR? I know it contradicts your vision of Optional but it is a non-breaking change. Does it make sense to have an alternative implementation to Optional? If you strictly against it, I won't even start.

@Kavan72, @Moribund7, @mvdwalle, @jwilmoth-ehs, @jpfinne, @Chrimle, @robinjhector Let me tag you and ask for your opinion, please. Does the idea of accessors make any sense to you? You may just downvote or upvote this message, but I would appreciate your comments as well.

Well, if using Optional is such a holy-war topic it always is an option to customize the template, right?

@MelleD
Copy link
Contributor

MelleD commented Jan 16, 2025

Hey @slobodator,

When it comes to the topic of optionality, there is a huge gap between “what it was introduced/intended for” and what (good) options and considerations arise from it. Sometimes things develop when you have good experiences. There is usually a way behind it:

Intention:
https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555
Road and discussion:
https://nipafx.dev/design-java-optional/

You see hard fights for a while and you see different projects which are also supporting more then only a return type (jackson, spring etc.). Additionally, everyone interprets the word “almost” differently.

Since there are discussions on this topic in the openJDK (Optional) as well. Because the idea and the envy are there or have been recognized.

For this reason, I'm waiting for the new features here:
https://openjdk.org/jeps/401
https://openjdk.org/jeps/8316779

Off Topic:
The only problem that I still don't understand and no one can explain to me.

The people who argue for the original jdk intent (use only in the return type) want to use this feature because the origin of this feature is that it is used in METHODS parameters for Spring Rest endpoints.
https://www.baeldung.com/spring-optional-path-variables#2-using-an-optional-parameter-type
This contradicts every principle of the intention :-) and then you should switch off the feature.

If a feature doesn't fit my expectations, ideas or anything else, I don't understand why people want to use it. And that's why I'm confused. But that's just how it is

I wish you much success with the hot topics and fights ;)

@slobodator
Copy link
Contributor Author

slobodator commented Jan 16, 2025

Hey @MelleD,

Even I have my own thoughts regarding Optional, I am trying to support all approaches as I explained my role at the company.

I guess ideally it would be to break compatibility (just to save efforts on supporting all possible combinations of flags at the mustache templates) and introduce two new feature toggles instead:

  • one for https://www.baeldung.com/spring-optional-path-variables#2-using-an-optional-parameter-type, something like useOptionalParameterType true/false. I am indifferent either to use or not
  • one for requests/respones something like useOptionalInModels. However, there are two (three -- do nothing) styles here no/fields/accessors
    • to make fields and their setters/getters optional -- as you did but this affects the current code
    • to introduce accessors -- as I'm suggesting with this discussion in non-breaking manner

Maybe I am still missing something, and even such a flexibility doesn't satisfy everyone, that's why I invited @Kavan72, @Moribund7, @mvdwalle, @jwilmoth-ehs, @jpfinne, @Chrimle, @robinjhector to the discussion.

P.S.

The only problem that I still don't understand and no one can explain to me.

It is only my assumption but as far as I got there were folks that

  • didn't mind using optional parameters at API
  • were quite confused that their getters became Optional unexpectedly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants