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

Entity model using interfaces does not generate Q classes correctly #82

Closed
damienhollis opened this Issue Jan 15, 2012 · 29 comments

Comments

Projects
None yet
3 participants
@damienhollis

Our JPA entity model uses interfaces extensively and it seems that QueryDSL does not handle them correctly.

Here is an example of our model:

@Entity
@Table(name = "USERS")
@org.hibernate.annotations.AccessType("field")
@org.hibernate.annotations.Proxy(proxyClass = User.class)
public class UserImpl extends EntityImpl implements User {

    @NaturalId(mutable = true)
    @Column(name = "USERNAME", nullable = false)
    private String username;

    @ManyToOne(cascade = {CascadeType.ALL}, fetch = FetchType.EAGER, targetEntity = PartyImpl.class)
    @JoinColumn(name = "PARTY_ID", nullable = false)
    private Party party;
}

@javax.persistence.Entity
@Table(name = "PARTY")
@org.hibernate.annotations.AccessType("field")
@org.hibernate.annotations.Proxy(proxyClass = Party.class)
public abstract class PartyImpl extends EntityImpl implements Party {

    @Column(name = "NAME", nullable = false)
    private String name;

}

Which generates a QUserImpl with the following field:

public final SimplePath<Party> party = createSimple("party", Party.class);

However, I believe it should look more like:

this.party = inits.isInitialized("party") ? new QPartyImpl(forProperty("party"), inits.get("party")) : null;

From reading the code it seems that the targetEntity attribute of the ManyToOne annotation is not being inspected. Is this type of model not supported? Or is there something else I need to do to make this work?

Regards,
Damien

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 16, 2012

Member

The ManyToOne relation is currently not taken into account.

Maybe I could simply change the APT processor for JPA to treat the property type of ManyToOne typed relatations as an entity type. Then you would get a Q-Type for Party.

Member

timowest commented Jan 16, 2012

The ManyToOne relation is currently not taken into account.

Maybe I could simply change the APT processor for JPA to treat the property type of ManyToOne typed relatations as an entity type. Then you would get a Q-Type for Party.

timowest added a commit that referenced this issue Jan 18, 2012

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Jan 23, 2012

I'm not sure whether that will resolve the issue or not. The Party is not actually an entity, the PartyImpl is. I think the APT processor will need to look at the @manytoone targetEntity attribute.

I'm not sure whether that will resolve the issue or not. The Party is not actually an entity, the PartyImpl is. I think the APT processor will need to look at the @manytoone targetEntity attribute.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 23, 2012

Member

Ok, the ManyToOne.targetEntity will then be used

Member

timowest commented Jan 23, 2012

Ok, the ManyToOne.targetEntity will then be used

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Jan 23, 2012

Sounds good. I should have made it clear that the targetEntity attribute is optional, so if it isn't present your original proposal would be correct.

Note, I haven't looked at what is generated for the OneToMany relationship but it should also use the targetEntity attribute, if it's present.

Sounds good. I should have made it clear that the targetEntity attribute is optional, so if it isn't present your original proposal would be correct.

Note, I haven't looked at what is generated for the OneToMany relationship but it should also use the targetEntity attribute, if it's present.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 23, 2012

Member

I just deployed a version with a fix as 2.3.0.BUILD-SNAPSHOT. Could you verify that it works as expected?

Member

timowest commented Jan 23, 2012

I just deployed a version with a fix as 2.3.0.BUILD-SNAPSHOT. Could you verify that it works as expected?

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Jan 23, 2012

I'll take a look today. When you say you've deployed a version, where do I find this deployed version?

I'll take a look today. When you say you've deployed a version, where do I find this deployed version?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 23, 2012

Member

It's accessible from our Maven repository : http://source.mysema.com/maven2/snapshots/

Deployed releases are synced to central repositories with a small delay, but our snapshots aren't.

Member

timowest commented Jan 23, 2012

It's accessible from our Maven repository : http://source.mysema.com/maven2/snapshots/

Deployed releases are synced to central repositories with a small delay, but our snapshots aren't.

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Jan 24, 2012

The code is generating correctly now - thanks. But it creates a new problem that I hadn't thought of.

Take the snippet below:
public class QUserImpl extends EntityPathBase {
....
public final com.walrus.model.domain.QPartyImpl party;
....
}

public class QPartyImpl extends EntityPathBase {
....
}

public User findUser(final Party party) {
    return getHibernateTemplate().execute(new HibernateCallback<User>() {
        @Nullable
        @Override
        public User doInHibernate(Session session) throws HibernateException, SQLException {
            com.mysema.query.jpa.hibernate.HibernateQuery query = new com.mysema.query.jpa.hibernate.HibernateQuery(session);
            return query.from(user).where(user.party.eq(party)).uniqueResult(user);
        }
    });

This no longer compiles because user.party is now a QPartyImpl (which is what I wanted) but the findUser method parameter is a Party (not a PartyImpl).

Unfortunately I can't cast a Party to a PartyImpl, so there is no way to make the current code work. Is it possible to make the type parameter of EntityPathBase be Party instead? I think that would resolve this particular issue, although I'm not sure what other impact it might have.

The code is generating correctly now - thanks. But it creates a new problem that I hadn't thought of.

Take the snippet below:
public class QUserImpl extends EntityPathBase {
....
public final com.walrus.model.domain.QPartyImpl party;
....
}

public class QPartyImpl extends EntityPathBase {
....
}

public User findUser(final Party party) {
    return getHibernateTemplate().execute(new HibernateCallback<User>() {
        @Nullable
        @Override
        public User doInHibernate(Session session) throws HibernateException, SQLException {
            com.mysema.query.jpa.hibernate.HibernateQuery query = new com.mysema.query.jpa.hibernate.HibernateQuery(session);
            return query.from(user).where(user.party.eq(party)).uniqueResult(user);
        }
    });

This no longer compiles because user.party is now a QPartyImpl (which is what I wanted) but the findUser method parameter is a Party (not a PartyImpl).

Unfortunately I can't cast a Party to a PartyImpl, so there is no way to make the current code work. Is it possible to make the type parameter of EntityPathBase be Party instead? I think that would resolve this particular issue, although I'm not sure what other impact it might have.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 24, 2012

Member

I will see what I can do.

Member

timowest commented Jan 24, 2012

I will see what I can do.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

I added now a fix which is available when you use the following APT processor: com.mysema.query.apt.hibernate.HibernateAnnotationProcessor

When using the Proxy annotation the meta model name is replaced with the proxy class name, so PartyImpl is mirrored into QParty and UserImpl into QUser.

Could you try out the latest snapshot and let me know if the solution works for you?

Member

timowest commented Feb 12, 2012

I added now a fix which is available when you use the following APT processor: com.mysema.query.apt.hibernate.HibernateAnnotationProcessor

When using the Proxy annotation the meta model name is replaced with the proxy class name, so PartyImpl is mirrored into QParty and UserImpl into QUser.

Could you try out the latest snapshot and let me know if the solution works for you?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 12, 2012

Member

It appears inheritance is not yet handled properly, but I am going to fix that as well.

Member

timowest commented Feb 12, 2012

It appears inheritance is not yet handled properly, but I am going to fix that as well.

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Feb 12, 2012

The change sounds good. I've tested it on our code and I get this error (I've removed the package name):

java.lang.IllegalStateException: No entity type for Entity

Entity is an interface that defines common entity methods (e.g. getId(), etc). There is a convenience implementation EntityImpl but it is annotated as a @MappedSuperclass and therefore has no @Proxy annotation.

It looks like your code is expecting all interfaces to have a corresponding class annotated with @entity and @Proxy. This isn't the case, we have interface like Entity and we also have other interfaces that define abstract concepts that are extended by one or more interfaces that represent domain objects (e.g. Taxable).

The change sounds good. I've tested it on our code and I get this error (I've removed the package name):

java.lang.IllegalStateException: No entity type for Entity

Entity is an interface that defines common entity methods (e.g. getId(), etc). There is a convenience implementation EntityImpl but it is annotated as a @MappedSuperclass and therefore has no @Proxy annotation.

It looks like your code is expecting all interfaces to have a corresponding class annotated with @entity and @Proxy. This isn't the case, we have interface like Entity and we also have other interfaces that define abstract concepts that are extended by one or more interfaces that represent domain objects (e.g. Taxable).

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 13, 2012

Member

The problem is the following: The Q-type instances you supply to the from calls need to refer to the Impl-types, since they are annotated with the JPA Entity annotations. The Q-type instances you use for properties should refer to the proxy types.

So I am not sure anymore if this can be fixed. Mirroring the interface types is tricky, since no annotations can be used to decide what the role of each type is.

Member

timowest commented Feb 13, 2012

The problem is the following: The Q-type instances you supply to the from calls need to refer to the Impl-types, since they are annotated with the JPA Entity annotations. The Q-type instances you use for properties should refer to the proxy types.

So I am not sure anymore if this can be fixed. Mirroring the interface types is tricky, since no annotations can be used to decide what the role of each type is.

timowest added a commit that referenced this issue Feb 13, 2012

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Feb 15, 2012

I understand the problem. I originally thought you'd tackle the problem slightly differently:

  • continue to mirror the Impl class
  • add an additional type parameter for the interface that is expected for properties

However, I don't understand your framework that well and in particular exactly what the mirror types responsibilities are. When I get a chance, I might fork a copy of your code and see if I can come up with any good ideas.

I understand the problem. I originally thought you'd tackle the problem slightly differently:

  • continue to mirror the Impl class
  • add an additional type parameter for the interface that is expected for properties

However, I don't understand your framework that well and in particular exactly what the mirror types responsibilities are. When I get a chance, I might fork a copy of your code and see if I can come up with any good ideas.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 15, 2012

Member

On Wed, Feb 15, 2012 at 3:01 AM, damienhollis <
reply@reply.github.com

wrote:

I understand the problem. I originally thought you'd tackle the problem
slightly differently:

  • continue to mirror the Impl class
  • add an additional type parameter for the interface that is expected for
    properties

Adding an additional type parameter would go too far. We can't do such a
thing for a minor release and I am not sure if this would be a good
direction anyway.

However, I don't understand your framework that well and in particular
exactly what the mirror types responsibilities are. When I get a chance, I
might fork a copy of your code and see if I can come up with any good ideas.

The Q-types are used to construct queries. They are used to refer to query
roots, properties and are also used to construct conditions and projections.


Reply to this email directly or view it on GitHub:
#82 (comment)

Timo Westkmper
Mysema Oy
+358 (0)40 591 2172
www.mysema.com

Member

timowest commented Feb 15, 2012

On Wed, Feb 15, 2012 at 3:01 AM, damienhollis <
reply@reply.github.com

wrote:

I understand the problem. I originally thought you'd tackle the problem
slightly differently:

  • continue to mirror the Impl class
  • add an additional type parameter for the interface that is expected for
    properties

Adding an additional type parameter would go too far. We can't do such a
thing for a minor release and I am not sure if this would be a good
direction anyway.

However, I don't understand your framework that well and in particular
exactly what the mirror types responsibilities are. When I get a chance, I
might fork a copy of your code and see if I can come up with any good ideas.

The Q-types are used to construct queries. They are used to refer to query
roots, properties and are also used to construct conditions and projections.


Reply to this email directly or view it on GitHub:
#82 (comment)

Timo Westkmper
Mysema Oy
+358 (0)40 591 2172
www.mysema.com

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Feb 15, 2012

I agree that changing the types parameters would be too much for a minor release.

I wonder whether the solution is somewhat simpler, maybe an additional type parameter is not needed, rather the existing one could be set to be the interface (if there is one, otherwise it could remain as is) rather than the impl. i.e

public class QUserImpl extends EntityPathBase<UserImpl> {...}

would become

public class QUserImpl extends EntityPathBase<User> {...}

I'm not sure of the complete impact of this change but it would solve my immediate problem (I think).

I've been look through your code to see if there was an easy way to quickly implement this to test the idea but I don't see one. The EntityType would need to be enhanced to return something different for getGenericName(...).

Does my approach make sense? What issues do you see? Can you think of an easy way to make the change?

By the way, I did understand what your Q-types were for, I was confused when you mentioned mirror types since I had seen TypeMirrors in the code and thought you were talking about them.

I agree that changing the types parameters would be too much for a minor release.

I wonder whether the solution is somewhat simpler, maybe an additional type parameter is not needed, rather the existing one could be set to be the interface (if there is one, otherwise it could remain as is) rather than the impl. i.e

public class QUserImpl extends EntityPathBase<UserImpl> {...}

would become

public class QUserImpl extends EntityPathBase<User> {...}

I'm not sure of the complete impact of this change but it would solve my immediate problem (I think).

I've been look through your code to see if there was an easy way to quickly implement this to test the idea but I don't see one. The EntityType would need to be enhanced to return something different for getGenericName(...).

Does my approach make sense? What issues do you see? Can you think of an easy way to make the change?

By the way, I did understand what your Q-types were for, I was confused when you mentioned mirror types since I had seen TypeMirrors in the code and thought you were talking about them.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 15, 2012

Member

If the type parameter refers to the interface type then you can't use the Q-types for querying, because the interface types are not entities.

Member

timowest commented Feb 15, 2012

If the type parameter refers to the interface type then you can't use the Q-types for querying, because the interface types are not entities.

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Feb 15, 2012

I'm only referring to the generic type not the Class parameter you supply when constructing the Q-type. So when actually creating a hibernate query, it would still be operating on the entity type.

However, I just tried hacking a generated Q-type to have this kind of signature and it causes compile errors within the Q-type because _super reference is no longer valid. This could be fixed if interfaces were used everywhere but in some cases the super class has not interface.

I'm only referring to the generic type not the Class parameter you supply when constructing the Q-type. So when actually creating a hibernate query, it would still be operating on the entity type.

However, I just tried hacking a generated Q-type to have this kind of signature and it causes compile errors within the Q-type because _super reference is no longer valid. This could be fixed if interfaces were used everywhere but in some cases the super class has not interface.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 15, 2012

Member

Having a type parameter that doesn't actually reflect the real type will cause some issues.

Member

timowest commented Feb 15, 2012

Having a type parameter that doesn't actually reflect the real type will cause some issues.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jun 29, 2012

Member

Closing this for now, as we couldn't figure out a proper fix

Member

timowest commented Jun 29, 2012

Closing this for now, as we couldn't figure out a proper fix

@timowest timowest closed this Jun 29, 2012

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Jul 5, 2012

Yeah, fair enough. I've been watching the querydsl google group and noticed nobody else has stumbled across this problem, which is a big surprise to us. For complex domain models, there really isn't any other option but to use interfaces to proxy the implementation classes when using hibernate - at least that was the case when we started using hibernate a while ago now. Maybe something has changed in recent years but I couldn't find any documentation saying that.

Good luck with the project and I'll keep watching in case you do manage to workout a fix.

Cheers,
Damien

Yeah, fair enough. I've been watching the querydsl google group and noticed nobody else has stumbled across this problem, which is a big surprise to us. For complex domain models, there really isn't any other option but to use interfaces to proxy the implementation classes when using hibernate - at least that was the case when we started using hibernate a while ago now. Maybe something has changed in recent years but I couldn't find any documentation saying that.

Good luck with the project and I'll keep watching in case you do manage to workout a fix.

Cheers,
Damien

@wrungel

This comment has been minimized.

Show comment
Hide comment
@wrungel

wrungel May 24, 2013

I had almost the same problem in my project. The difference is that the targetEntity annotation was always missing and we used orm.xml to specify it.
To solve the problem, I have implemented a new generator for Q-classes from scratch.

wrungel commented May 24, 2013

I had almost the same problem in my project. The difference is that the targetEntity annotation was always missing and we used orm.xml to specify it.
To solve the problem, I have implemented a new generator for Q-classes from scratch.

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Oct 18, 2013

Hi Timo,

I'm looking at using querydsl again but I noticed you reverted the fix that looked at the ManyToOne.targetEntity. When you originally fixed this, I said I'd come across another problem because I couldn't cast a Party to a PartyImpl. However I have found a way to unproxy the hibernate object, so I can work around this problem.

The problem now is that you reverted all your changes, as far as I can tell. Can you reinstate the fix that looked at the targetEntity?

Regards,
Damien

Hi Timo,

I'm looking at using querydsl again but I noticed you reverted the fix that looked at the ManyToOne.targetEntity. When you originally fixed this, I said I'd come across another problem because I couldn't cast a Party to a PartyImpl. However I have found a way to unproxy the hibernate object, so I can work around this problem.

The problem now is that you reverted all your changes, as far as I can tell. Can you reinstate the fix that looked at the targetEntity?

Regards,
Damien

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 22, 2013

Member

@damienhollis Ok, I will give it a try later.

Member

timowest commented Oct 22, 2013

@damienhollis Ok, I will give it a try later.

@timowest timowest reopened this Oct 23, 2013

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 23, 2013

Member

@damienhollis Could you try with the latest version?

Member

timowest commented Oct 23, 2013

@damienhollis Could you try with the latest version?

@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Oct 23, 2013

Where do I find the latest version? I looked here: http://source.mysema.com/maven2/snapshots/ but it doesn't contain 3.x.x versions.

Where do I find the latest version? I looked here: http://source.mysema.com/maven2/snapshots/ but it doesn't contain 3.x.x versions.

@timowest

This comment has been minimized.

Show comment
Hide comment
@damienhollis

This comment has been minimized.

Show comment
Hide comment
@damienhollis

damienhollis Oct 24, 2013

The code generates properly now - thanks!

The code generates properly now - thanks!

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 17, 2013

Member

Released in 3.3.0.BETA1

Member

timowest commented Nov 17, 2013

Released in 3.3.0.BETA1

@timowest timowest added this to the 3.3.0 milestone Apr 13, 2014

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