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][spring] Required properties of a model should be made mandatory in constructor #9789

Closed
mimkorn opened this issue Jun 16, 2021 · 4 comments · Fixed by #14822
Closed

Comments

@mimkorn
Copy link

mimkorn commented Jun 16, 2021

Problem

The constructors of the generated models with the spring generator have no builders, empty constructors. The fact that a property is required is only in an annotation that I do not think has any other effect than informing the reader.

Describe the solution you'd like

IMHO there should be a constructor that would have all the required properties in it and no constructor without it. This way you'd enforce setting those up. Otherwise what stops you from sending responses from the server without required properties?

Describe alternatives you've considered

using lombok to create builders and constructors would be nice in general.

Is there some way generated server stub ensures adhering to the contract in the question of required attribute?

@ybelenko ybelenko changed the title [REQ] Required properties of a model should be made mandatory in constructor [REQ][spring] Required properties of a model should be made mandatory in constructor Jun 16, 2021
@RockyMM
Copy link
Contributor

RockyMM commented Apr 6, 2022

Would not javax.validation take care of this req? I am not sure, I am just asking.

However, your request makes sense.

Lombok should not be used as it is adding yet another dependency to the generated stuff, and it has it's own... drawbacks.

alucas added a commit to alucas/openapi-generator that referenced this issue Mar 1, 2023
borsch added a commit that referenced this issue Mar 1, 2023
* Add constructor with required parameter for spring

Fix #9789

* [Java][Spring] constructor with required args

---------

Co-authored-by: Oleh Kurpiak <[email protected]>
@OlliL
Copy link

OlliL commented May 6, 2023

There is no way to enforce "is required". This request is pointless imho.

  • removing the default constructor makes using many maping frameworks impossible
  • removing the default constructor breaks the POJO spec
  • just marking the default constructor is pointless as it can still be used but also clotters the code with warnings
  • adding a constructor with only-required parameters but w/o nullcheck does not prevent you from handing in null values
  • you still can set properties to null afterwards

It might add a way of adding more convinience using that POJO by having an additional constructor - but thats all, nothing else.

To really enforce "is required" you would need to:

  • make class final
  • make default constructor private
  • add null checks to constructor with only-required parameters
  • add null checks to all setter-methods

but then you get a POJO which can't be used with mapping or serialization/deserialization frameworks.... so stop attempting something you can't reach

@frankjkelly
Copy link

Perhaps I am missing a constraint but what challenges/blockers would have to be solved to be able to add null checks on setters? (The setters could then be re-used in the constructor or builder).

@OlliL
Copy link

OlliL commented May 8, 2023

Yeah this probably could be done. On the other hand the default constructor could never be removed I guess. That said there will always be a chance of ending up with null values for required-props.
So yes - you can make the lifes of a software developer easier by having that additional constructor so when constructing this class manually you can anticipate what properties needs to be filled. But - on the other side you can't rely on this constructor being used everywhere so you have to check if all required props are set anyway when consuming that object. It adds convenience but no assurance imho.

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

Successfully merging a pull request may close this issue.

5 participants