Query clone support with pre-existing entityManager/connection #672

Closed
TuomasKiviaho opened this Issue Feb 24, 2014 · 6 comments

Comments

Projects
None yet
2 participants
@TuomasKiviaho
Contributor

TuomasKiviaho commented Feb 24, 2014

I've used so far the jpaSqlQuery.clone(entityManager) method and thought to give it a try on the sql side as well. First of all it was missing from the mysql specific implementation altogether, but luckily it was present on the more generic SQLQuery.

On the JPA side it was easy enough (although a bit irritating) to carry around the entity manager as well but to get a hold to the same connection that 's inside the original sqlQuery proved to be problematic.

Would it be possible to introduce a new clone() method on the interface level and reintroduce the pre-existing clone(entityManager/connection) as abstract on both of the AbstractSQLQuery implementations. This would also ensure that all of the implementations implement also the cloning.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 24, 2014

Member

Here is a summary of what you suggest:

  • com.mysema.query.sql.AbstractSQLQuery
    • public abstract SQLQuery clone(Connection conn);
  • com.mysema.query.jpa.sql.AbstractJPASQLQuery
    • public abstract JPASQLQuery clone(EntityManager entityManager);
  • com.mysema.query.jpa.hibernate.sql.AbstractHibernateSQLQuery
    • public abstract HibernateSQLQuery clone(Session session);

In addition to this implementation of the missing clone methods in the subclasses.

The interfaces for SQL queries, e.g. SQLCommonQuery, are more meant to enforce consistent naming for queries and subqueries.

Could you confirm whether I got it right?

Member

timowest commented Feb 24, 2014

Here is a summary of what you suggest:

  • com.mysema.query.sql.AbstractSQLQuery
    • public abstract SQLQuery clone(Connection conn);
  • com.mysema.query.jpa.sql.AbstractJPASQLQuery
    • public abstract JPASQLQuery clone(EntityManager entityManager);
  • com.mysema.query.jpa.hibernate.sql.AbstractHibernateSQLQuery
    • public abstract HibernateSQLQuery clone(Session session);

In addition to this implementation of the missing clone methods in the subclasses.

The interfaces for SQL queries, e.g. SQLCommonQuery, are more meant to enforce consistent naming for queries and subqueries.

Could you confirm whether I got it right?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Feb 25, 2014

Contributor

That's the part one. I think that AbstractSQLSubQuery could also benefit from cloning. As a side note: SQLServerSubQuery doesn't seem to be included to SQLServerQueryFactory).

The second part of the request would be to clone without knowing anything that is laying below the bonnet (my original use case). I know this suggestion contradicts a bit what you stated but due to the lack of common ancestor class this is the best that I came up with.

com.mysema.query.sql.SQLCommonQuery<Q extends SQLCommonQuery<Q>>
  public Q clone();
Contributor

TuomasKiviaho commented Feb 25, 2014

That's the part one. I think that AbstractSQLSubQuery could also benefit from cloning. As a side note: SQLServerSubQuery doesn't seem to be included to SQLServerQueryFactory).

The second part of the request would be to clone without knowing anything that is laying below the bonnet (my original use case). I know this suggestion contradicts a bit what you stated but due to the lack of common ancestor class this is the best that I came up with.

com.mysema.query.sql.SQLCommonQuery<Q extends SQLCommonQuery<Q>>
  public Q clone();
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 27, 2014

Member

What would the semantics of the general clone()? Create an attached version or a detached one?

Member

timowest commented Feb 27, 2014

What would the semantics of the general clone()? Create an attached version or a detached one?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 20, 2014

Member

@TuomasKiviaho Would you be interested to try to create a pull request for this?

Member

timowest commented Mar 20, 2014

@TuomasKiviaho Would you be interested to try to create a pull request for this?

TuomasKiviaho added a commit to TuomasKiviaho/querydsl that referenced this issue Apr 13, 2014

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Apr 13, 2014

Contributor

Sorry about the long delay. Here a pull request #713

It wasn't as straight forward as I'd first though and I guess I over-accomplished a bit by pulling out common ancestors for both detached and projectable sql queries so that I could get rid of clear duplicates (sql/jpa.AbstractSQLQuery -> ProjectableSQLQuery and sql.AbstractSQLSubQuery -> DetachableSQLQuery). I also refactored out constants from previous serializer instantiation that were preserved among internal state.

I guess you were referring with the semantics to this change that you posted to me in another issue 69ea056

I chose the diplomatic route and used the same approach although I think it would be wiser to check if the templating is the default one and only then apply the JPAProvider.getTemplates(). With the current approach it would be impossible to use custom queryhandler coherently since it would be lost in the cloning process.

Contributor

TuomasKiviaho commented Apr 13, 2014

Sorry about the long delay. Here a pull request #713

It wasn't as straight forward as I'd first though and I guess I over-accomplished a bit by pulling out common ancestors for both detached and projectable sql queries so that I could get rid of clear duplicates (sql/jpa.AbstractSQLQuery -> ProjectableSQLQuery and sql.AbstractSQLSubQuery -> DetachableSQLQuery). I also refactored out constants from previous serializer instantiation that were preserved among internal state.

I guess you were referring with the semantics to this change that you posted to me in another issue 69ea056

I chose the diplomatic route and used the same approach although I think it would be wiser to check if the templating is the default one and only then apply the JPAProvider.getTemplates(). With the current approach it would be impossible to use custom queryhandler coherently since it would be lost in the cloning process.

timowest added a commit that referenced this issue Apr 14, 2014

Merge pull request #713 from TuomasKiviaho/Clone-672
Query clone support with pre-existing entityManager/connection #672

@timowest timowest added the fixed label Apr 14, 2014

@timowest timowest added this to the 3.3.3 milestone Apr 15, 2014

@timowest timowest modified the milestone: 3.3.3 Apr 30, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 2, 2014

Member

Released in 3.3.3

Member

timowest commented May 2, 2014

Released in 3.3.3

@timowest timowest closed this May 2, 2014

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