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

Relax default/no-arg constructor requirement #338

Closed
m-reza-rahman opened this issue Dec 4, 2021 · 11 comments
Closed

Relax default/no-arg constructor requirement #338

m-reza-rahman opened this issue Dec 4, 2021 · 11 comments

Comments

@m-reza-rahman
Copy link

m-reza-rahman commented Dec 4, 2021

Especially in light of Records, should Jakarta Persistence relax the default constructor requirement for entities and embeddables? While I am not very supportive of broad use of Records as entities, embeddables or even part of a domain model (to me the natural fit for Records is as a DTO), I can see certain use cases such as reporting and the query parts of CQRS.

I have also seen some infrequent cases where having to add a default constructor needlessly pollutes the object/domain model.

Reza Rahman
Jakarta EE Ambassador, Author, Blogger, Speaker

Please note views expressed here are my own as an individual community member and do not reflect the views of my employer.

@OndroMih
Copy link

OndroMih commented Dec 5, 2021

I never understood why it's required that those classes implement a no-arg constructor. If there's a reliable technical solution how to relax this, I'm strongly in favor. It would allow more flexible programming model for users, who would like to use custom constructors in their code. And I believe there is a solution, we just need to talk about it and refine it.

From implementation point of view it makes sense - the implementation needs to create a proxy that inherits from the class and needs to call the super constructor. Calling a single no-arg constructor is simple, there are no dependencies, it shouldn't raise an exception. However, I believe there are also reliable solutions to instantiating a subclass even without a no-arg constructor. And even without some hacks that some frameworks use, if we keep some limitations.

An example of a relaxed limitation that makes sense to me is to allow classes without no-arg constructors, but mandates that there's a constructor that allows null values for all arguments (or a default value for primitive types).

Technically, it would be possible to instantiate a subclass, it would just be a bit more complicated to do bytecode generation for the subclass. If there are mote constructors in the superclas, the specification could also request that one of them is selected as the one for creating proxy subclasses, e.g. via a specific annotation. I would add that annotation to Jakarta Annotations or the new Jakarta Commons specification, because it could be reused by other specifications like CDI, which also request no-arg constructor now.

@rbygrave
Copy link

rbygrave commented Dec 5, 2021

record types

Just to say that record types are a great fit for @EmbeddedId but in my biased opinion there are plenty of cases where we don't want the default constructor requirement.

relax the default constructor requirement for entities and embeddables?

Well it is certainly possible as I've done it myself with Ebean ORM but as I see it there is some implementation complexity and implications and I feel this is somewhat academic until the Hibernate and Eclipselink developers add in their comments and thoughts.

So I'll add some thoughts on this with the view that I've actually done it myself and personally believe it is an important feature but with the view that it's all moot until the Hibernate and Eclipselink developers have their say wrt implementation complexity.

bit more complicated to do bytecode generation for the subclass

Yes, if I recall correctly wrt how this was done with Ebean ORM this does get more complex (to the point that we decided to remove support for dynamic proxies). That is, in the case of Ebean ORM it originally supported both dynamic proxies and enhancement and ultimately we got rid of the support for dynamic proxies due to the complexity involved here. Noting that this was about 10+ years ago when we made that decision so my memory on the details has waned - way too many coffees have come and gone since then. With enhancement we have the simplicity of knowing everything in the inheritance hierarchy is enhanced. In the case of dynamic proxies, at least with how Ebean used to do it we couldn't get that simplicity, to the point that we took the path of removing dynamic proxies and hence mandated the use of enhancement.

My throw out there opinionated comment is - I believe this requirement is likely to be much easier to actually implement under the conditions that entities are enhanced. All the JPA vendors support the enhancement option and one possible path here is the default constructor requirement be removed under the condition that enhancement is used.

In summary, I think the questions going to JPA implementation developers could be:

Q: Is it worthwhile to remove the default constructor requirement under the condition of using enhancement ?

Q: Is it worthwhile to remove the default constructor requirement under all conditions ?

... with "worthwhile" here relating to the expected cost of implementation complexity against the feature benefit. My thought is that under the conditions of enhancement I believe this is a relatively easy restriction to remove, but in terms of dynamic proxies if I recall correctly this isn't so easy and adds quite a lot of implementation cost/complexity to the point that it might be hard to justify.

My 2c, hopefully useful.
Cheers, Rob.

@beikov
Copy link

beikov commented Dec 6, 2021

Jakarta Persistence is a standard and this is something that AFAIU no implementation supports at this time, so I am a bit unsure it makes sense to discuss this here. The spec and TCK AFAIU can't mandate anything for which there no compatible implementation exists, so if anyone wants this feature I'd suggest to start discussing/prototyping this in Hibernate or EclipseLink. IMO we first need some experience how this works out in practice before talking about standardization.
FWIW the Hibernate team is actively working on something in this area for 6.0, but the feature will take some more time.

@m-reza-rahman
Copy link
Author

m-reza-rahman commented Dec 6, 2021

Specifications need not be purely implementation driven (in fact I have never seen them to be in many years certainly including in the JCP - an organization some people claim is too vendor driven). Of course this is work that ought to be undertaken in implementations at some point, but it would be very valuable to have some input from developers that use Jakarta Persistence in applications here in the meantime. One should hope implementations are always keeping an ear to the ground to input here as well and not just in their own implementation space.

@AleksNo
Copy link

AleksNo commented Dec 6, 2021

Hello,

removing the no-arg constructor requirement would have one big advantage: better interoperability with other programming languages. In Kotlin it is not possible to make a class with a no-arg contructor. They need a special compiler plugin to make entities compatible with JPA:
https://kotlinlang.org/docs/no-arg-plugin.html

And then there is Project Valhalla. AFAIR inline classes will also not have a no-arg constructor. Without this requirement maybe they will be usable in JPA without hassle as entities.

Cheers

@m-reza-rahman m-reza-rahman changed the title Relax default constructor requirement Relax default/no-arg constructor requirement Dec 6, 2021
@andyjefferson
Copy link

FWIW DataNucleus has never had such a requirement, since it uses bytecode enhancement and adds a default constructor (when not provided) to each entity.

@gavinking
Copy link
Contributor

We've done some thinking about this lately, and it seems to me that a "record entity" just isn't a very useful construct for the following reasons:

On the other hand, a record is a perfectly legitimate way to implement an @Embeddable and @IdClass type with no associations, and we should add this to JPA.

I'm happy to take a stab at specc'ing this.

@trajano
Copy link

trajano commented Aug 9, 2023

I agree with @gavinking for every practical case for JPA entities, they need to be mutable and records are not. The presence of a protected or public no-arg constructor for the JPA engine to construct makes sense.

Note I had specified protected which is allowed by Hibernate at least (I am not sure if it is in the spec itself). This will allow us to provide something like this (note using Lombok)

@Entity
@Data
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Table(
    uniqueConstraints = {
      @UniqueConstraint(
          columnNames = {"myLookupKey"})
    })
public class MyEntity {
  @Id
  @Column(columnDefinition = "BINARY(16)")
  @GeneratedValue(strategy = GenerationType.UUID)
  private UUID id;

  /** This is the business key to look up the data. This is the natural key for the entity. */
  @Column(nullable = false)
  @Setter(AccessLevel.PROTECTED)
  @EqualsAndHashCode.Include
  private String myLookupKey;

  /**
   * Public constructor that requires the `natural key` and maybe other required data to be provided.
   */
  public MyEntity(@NotNull final String myLookupKey) {

    this.myLookupKey = myLookupKey;
  }

@tofflos
Copy link

tofflos commented Aug 9, 2023

I'm out of my depth here but asking API users to allow mutability seems very reasonable for this type of API. But asking API users to allow uninitialized objects that may contain fields that have no natural defaults is another story as it transforms what is a challenge for the API implementer (i.e. EclipseLink and Hibernate) to a challenge for the API user (i.e. me) - and I'm much less qualified to deal with it. So if it this is a thing that could be solved by API implementers without breaking their backs that would be nice. ❤

@gavinking
Copy link
Contributor

Please see #379 and #428 which are relevant to this discussion. IMO, these changes address the needs described above, and once #428 is merged, we can close this.

@gavinking
Copy link
Contributor

Please see #379 and #428 which are relevant to this discussion. IMO, these changes address the needs described above, and once #428 is merged, we can close this.

Those changes have now been merged, so I'm going to close this issue.

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

No branches or pull requests

9 participants