Re: ExecuteListener's instance creation

[Lukas Eder]

Hi Sergey

Thanks for this report. I will CC this E-Mail to the user group as
there had been a recent, related discussion with Aaron Digulla
concerning this part of the logic. The related discussion was about
performance, and Aaron suggested to post a patch idea here on the user
group. Maybe that patch would also resolve your class-loading issue.

I will consider your suggestion later this week

Cheers
Lukas

2012/7/4 Sergey Epik <serge...@gmail.com>:
> Hello Lukas,
>
> We had a problem to use ExecuteListener's in OSGI environment.
>
> JOOQ creates ExecuteListener instances in Util.getListener method and keep
> them in private static final Map.
> In our application jOOQ library and client code are in separate bundles, so
> they have different classloaders (according to OSGI).
> So Util class is unable to create instance of ExecuteListener from another
> bundle using Class.forName.
>
> We found the following solution:
> 1. Add private List<ExecuteListener> executeListeners;
> with setters/getters to Factory.java
> 2.
> Add
>>
>> List<ExecuteListener> getExecuteListeners();
>>
>> void setExecuteListeners(List<ExecuteListener> listeners);
>
> to Configuration.java
> 3. Remove from Util.java
>>
>> /**
>> * A cache for {@link ExecuteListener} classes
>> */
>> private static final Map<String, Class<?>> EXECUTE_LISTENERS = new
>> ConcurrentHashMap<String, Class<?>>();
>
> 4. Change Util.getListeners:
>>
>> static final List<ExecuteListener> getListeners(Configuration
>> configuration) {
>> List<ExecuteListener> result = new ArrayList<ExecuteListener>();
>>
>> if (!FALSE.equals(configuration.getSettings().isExecuteLogging()))
>> {
>> result.add(new StopWatchListener());
>> result.add(new LoggerListener());
>> }
>>
>> if (configuration.getExecuteListeners() != null) {
>> result.addAll(configuration.getExecuteListeners());
>> }
>> return result;
>> }
>
>
> These changes allow us to instantiate ExcecuteListener instances outside
> jOOQ and inject them to Factory implementation.
>
>
> Best regards,
> Sergey Epik

[digulla]

> We found the following solution:
> 1. Add private List<ExecuteListener> executeListeners;
> with setters/getters to Factory.java
> 2.
> Add
>>
>>     List<ExecuteListener> getExecuteListeners();
>>
>>     void setExecuteListeners(List<ExecuteListener> listeners);
>
> to  Configuration.java

I have written a patch which uses composition over inhertiance. All objects now have a reference to the configuration instead of inheriting from it. That makes a lot of code much more simple and allows to get rid of many constructors and some methods. The patch for ExecuteListeners would fit perfectly into the new approach.

You can find the patch here: https://github.com/digulla/jOOQ/commit/96fa3b5e918dff6e5aa57a940a3490df8db05147

Note that it depends on the ConnectionProvider which is in this commit: https://github.com/digulla/jOOQ/commit/182d8a293c7633d1e31f64fa6a108813f0a89368

The second commit allows to use any kind of provider to get a connection - at lest it should. Right now, the source of the connection leaks into several places in the jOOQ codebase (DataSourceConnection, ConnectionProxy, DataSourceStatement, ...). My approach avoid this. You just ask the Configuration for the connection provider which gives you the connection.

The only open end here is that I don't know where jOOQ closes the connections that it got from the DataSource but Lukas can probably add the call to ConnectionProvider.close() in a few minutes.

Now back to the meat. Instead of inheriting from Configuration, I pass a reference to a Configuration around, possibly wrapping an existing Configuration when necessary. So instead of 5-6 constructors, the DB specific factories just have two:

    public H2Factory(Configuration configuration) {

        super(new DefaultConfiguration(configuration, SQLDialect.H2));

    }

    public H2Factory() {

        super(new DefaultConfiguration(SQLDialect.H2));

    }

In the default constructor, you could also use a value instance of Configuration. But since I'm unsure whether this patch has any chance to be included in the release, I didn't want to spend move than 8 hours on it.

There are these questions left:

1. When do I pass the configuration on and when do I clone?

2. When I clone, how deep do I clone? Should I clone the ConnectionProvider, too? How about the data map? Settings?

3. Factory has a method "attach" which I don't understand. The original code was:

    attachable.attach(this);

Now, it is:

    attachable.attach(configuration);

which might be wrong.

Comments?

Regards,

A. Digulla

[Lukas Eder]

Hi Aaron,

Thanks for these contributions.

> I have written a patch which uses composition over inhertiance. All objects
> now have a reference to the configuration instead of inheriting from it.
> That makes a lot of code much more simple and allows to get rid of many
> constructors and some methods. The patch for ExecuteListeners would fit
> perfectly into the new approach.

So you agree with Sergey's contribution?

> You can find the patch here:
> https://github.com/digulla/jOOQ/commit/96fa3b5e918dff6e5aa57a940a3490df8db05147

Looks interesting at first sight. However...

> Note that it depends on the ConnectionProvider which is in this commit:
> https://github.com/digulla/jOOQ/commit/182d8a293c7633d1e31f64fa6a108813f0a89368

Would it be possible to create one branch / pull-request per idea? I
would like to have a look at your improvements to the Configuration /
Factory relationship without the side-effects of a ConnectionProvider.

I'm still not convinced by this ConnectionProvider. jOOQ users should
not bother with an additional non-JDBC type that does almost the same
as a JDBC DataSource when they already have JDBC Connections and
DataSources. Even if we'd introduce this, it should not be the only
way to interact with jOOQ's Factory. The existing operation modes
involving Connection and DataSource must be maintained.

Regarding your JDBC compatibility concerns: The JDK 7 is the last one
to contain an incompatible API upgrade (from JDBC 4.0 to 4.1). With
Java 8 extension methods, JDBC 4.2 or 5.0 or whatever it will be
called can be expected to be compatible with JDBC 4.1. While you argue
that JDBC API upgrades had been incompatible in the past and that this
had led to issues with your source code, you promote incompatible
upgrades of the jOOQ API instead. I'm sure, most users will find the
JDBC API upgrades less painful.

At the same time, I have fixed #1498. With this, it should be possible
to compile (and run) jOOQ and the jOOQ Console against JDBC 4.1. Did
you try it?
https://sourceforge.net/apps/trac/jooq/ticket/1498

> The second commit allows to use any kind of provider to get a connection -
> at lest it should. Right now, the source of the connection leaks into
> several places in the jOOQ codebase (DataSourceConnection, ConnectionProxy,
> DataSourceStatement, ...). My approach avoid this. You just ask the
> Configuration for the connection provider which gives you the connection.

jOOQ's internals can live with a "leaking connection source", I think.
The current implementation seems robust enough to me. But client code
should not be bothered with leaks of jOOQ's internals (=
ConnectionProvider). Some users on this group wouldn't be happy with
that... Some sample discussions:

- https://groups.google.com/d/topic/jooq-user/q6Yf7voPaqQ/discussion
- https://groups.google.com/d/topic/jooq-user/SPq6lKX-BtM/discussion

> The only open end here is that I don't know where jOOQ closes the
> connections that it got from the DataSource but Lukas can probably add the
> call to ConnectionProvider.close() in a few minutes.

DataSourceStatement closes DataSource-generated connections on
Statement.close() invocation. That is the typical moment when you'd
return (close) a Connection that was obtained from a DataSource.
Standalone Connections are never closed by jOOQ.

> Now back to the meat. Instead of inheriting from Configuration, I pass a
> reference to a Configuration around, possibly wrapping an existing
> Configuration when necessary. So instead of 5-6 constructors, the DB
> specific factories just have two:
>
> public H2Factory(Configuration configuration) {
> super(new DefaultConfiguration(configuration, SQLDialect.H2));
> }
> public H2Factory() {
> super(new DefaultConfiguration(SQLDialect.H2));
> }

Yes, that makes sense. I can see how composition instead of
inheritance will improve the overall design here. Note though, your
suggested changes in their current state are too fundamental to be
merged as a whole for the jOOQ 2.x stream. Incompatible API changes
will have to be postponed to jOOQ 3.0... I'll track this as #1533:
https://sourceforge.net/apps/trac/jooq/ticket/1533

Maybe there is a compatible way to implement this, though? I could
give it a shot, myself. Of course, this would lead to even more
constructors. The existing constructors could remain, as convenience
constructors.

> But since I'm unsure whether this patch has any chance to be
> included in the release, I didn't want to spend move than 8 hours on it.

No problem.

> There are these questions left:
>
> 1. When do I pass the configuration on and when do I clone?
> 2. When I clone, how deep do I clone? Should I clone the ConnectionProvider,
> too? How about the data map? Settings?

I'm not sure why cloning of Configuration attributes should be done...?

> 3. Factory has a method "attach" which I don't understand. [...]

I hope that this "attach" method will become obsolete at some point.
Some rather remote org.jooq.QueryPart and org.jooq.Store objects need
live Connections to fulfill their API. attach() takes care of that,
recursively. This is a legacy "feature" from pre-2.0 implementations

> The original code
> was:
>
> attachable.attach(this);
>
> Now, it is:
>
> attachable.attach(configuration);
>
> which might be wrong.

Seems right to me.

[Lukas Eder]

Hi Sergey,

I've had a closer look to your suggested contribution. This would
break existing API where ExecuteListeners are registered in Settings,
with the possibility of loading ExecuteListener configuration from an
XML configuration file. Settings is the right place for
ExecuteListeners.

Do you see any other solution that would remain compatible?

Cheers
Lukas

2012/7/5 Lukas Eder <lukas...@gmail.com>:

[digulla]

Am Donnerstag, 5. Juli 2012 19:45:09 UTC+2 schrieb Lukas Eder:

> I have written a patch which uses composition over inhertiance. All objects
> now have a reference to the configuration instead of inheriting from it.
> That makes a lot of code much more simple and allows to get rid of many
> constructors and some methods. The patch for ExecuteListeners would fit
> perfectly into the new approach.

So you agree with Sergey's contribution?

Yes.

 

> You can find the patch here:
> https://github.com/digulla/jOOQ/commit/96fa3b5e918dff6e5aa57a940a3490df8db05147

Looks interesting at first sight. However...

> Note that it depends on the ConnectionProvider which is in this commit:
> https://github.com/digulla/jOOQ/commit/182d8a293c7633d1e31f64fa6a108813f0a89368

Would it be possible to create one branch / pull-request per idea? I
would like to have a look at your improvements to the Configuration /
Factory relationship without the side-effects of a ConnectionProvider.

If you look at the code, you'll see that it doesn't make sense. It would be simple to get rid of the ConnectionProvider since only Configuration uses it. But if I hadn't added it, there would be no chance for you to even look at the idea.

As it is, see it as a proof of concept that a) it's possible, b) the impact is low key and c) it would solve many problems (see below).

If you really don't want it, well, freedom is just one fork away. But I've had excellent results with this approach in the past and I really don't understand why people don't get it.

Every application out there and every framework has their own GREAT IDEA(TM) how to get a connection from JDBC. No one uses the JDBC API directly; they use Spring or Hibernate or Transaction managers or JNDI or whatnot.

Yes, it would be great if they simply gave you the connection. But they don't. Everyone tries to be smart. So my solution is to wrap all this b****** in a single place to prevent frameworks and applications from leaking into jOOQ.

 

I'm still not convinced by this ConnectionProvider. jOOQ users should
not bother with an additional non-JDBC type that does almost the same
as a JDBC DataSource when they already have JDBC Connections and
DataSources. Even if we'd introduce this, it should not be the only
way to interact with jOOQ's Factory. The existing operation modes
involving Connection and DataSource must be maintained.

They are already maintained. You wrote about 80 constructors where you should have written only 8. If you look at the code, Configuration has become the owner of the connection no matter where it comes from so everything now happens in a single place.

If you want to add JNDI support, instead of adding yet another constructor to Factory and everything that extends it, you write a JndiConnectionProvider. If you have Sping, you can write a JdbcTemplateConnectionProvider or use the DefaultConnectionProvider. If you use Hibernate, ... you get the idea.

As you can see, your approach is to add N*M constructors while my approach uses a single constructor and an interface which defines how jOOQ communicates with the outside world.

Having JDBC API compatibility is just an additional bonus.

 

At the same time, I have fixed #1498. With this, it should be possible
to compile (and run) jOOQ and the jOOQ Console against JDBC 4.1. Did
you try it?
https://sourceforge.net/apps/trac/jooq/ticket/1498

No, and I'm not sure when or if I'll have the time.

> The second commit allows to use any kind of provider to get a connection -
> at lest it should. Right now, the source of the connection leaks into
> several places in the jOOQ codebase (DataSourceConnection, ConnectionProxy,
> DataSourceStatement, ...). My approach avoid this. You just ask the
> Configuration for the connection provider which gives you the connection.

jOOQ's internals can live with a "leaking connection source", I think.
The current implementation seems robust enough to me. But client code
should not be bothered with leaks of jOOQ's internals (=
ConnectionProvider). Some users on this group wouldn't be happy with
that...

OO imposes some limitations on us and Java even more so. Someone thought that interfaces are a great idea and now, we have to live with it. But is it really better to add a new set of constructors for every connection source?

How did you plan to add JNDI support to jOOQ?

 

> The only open end here is that I don't know where jOOQ closes the
> connections that it got from the DataSource but Lukas can probably add the
> call to ConnectionProvider.close() in a few minutes.

DataSourceStatement closes DataSource-generated connections on
Statement.close() invocation. That is the typical moment when you'd
return (close) a Connection that was obtained from a DataSource.

Okay. Just call configuration.getConfigurationProvider().close() instead. It's an empty method for standalone connections.

> Now back to the meat. Instead of inheriting from Configuration, I pass a
> reference to a Configuration around, possibly wrapping an existing
> Configuration when necessary. So instead of 5-6 constructors, the DB
> specific factories just have two:
>
>     public H2Factory(Configuration configuration) {
>         super(new DefaultConfiguration(configuration, SQLDialect.H2));
>     }
>     public H2Factory() {
>         super(new DefaultConfiguration(SQLDialect.H2));
>     }

Yes, that makes sense. I can see how composition instead of
inheritance will improve the overall design here. Note though, your
suggested changes in their current state are too fundamental to be
merged as a whole for the jOOQ 2.x stream. Incompatible API changes
will have to be postponed to jOOQ 3.0... I'll track this as #1533:
https://sourceforge.net/apps/trac/jooq/ticket/1533

Maybe there is a compatible way to implement this, though? I could
give it a shot, myself. Of course, this would lead to even more
constructors. The existing constructors could remain, as convenience
constructors.

You can create a configuration in the existing constructors and deprecate them. I didn't because I wanted to demonstrate the effect.

That's why I didn't create a pull request; the code in my branch is just a spike to prove that I know what I'm talking about.

 

> There are these questions left:
>
> 1. When do I pass the configuration on and when do I clone?
> 2. When I clone, how deep do I clone? Should I clone the ConnectionProvider,
> too? How about the data map? Settings?

I'm not sure why cloning of Configuration attributes should be done...?

That depends on the life cycle of the configuration. If you pass a Oracle configuration to H2Factory, the dialect must be replaced. If you simply changed it without cloning, Oracle won't be happy later.

There might be similar problems with data and settings but I don't understand what they do, so that's something which you must decide.

 

> 3. Factory has a method "attach" which I don't understand. [...]

I hope that this "attach" method will become obsolete at some point.
Some rather remote org.jooq.QueryPart and org.jooq.Store objects need
live Connections to fulfill their API. attach() takes care of that,
recursively. This is a legacy "feature" from pre-2.0 implementations

In that case, my change is correct but maybe it would be better to add a counter to the DataSourceConnectionProvider to make sure the connection is only closed after the last piece of code is done with it.

Regards,

A. Digulla

[digulla]

Am Donnerstag, 5. Juli 2012 19:55:21 UTC+2 schrieb Lukas Eder:

Hi Sergey,

I've had a closer look to your suggested contribution. This would
break existing API where ExecuteListeners are registered in Settings,
with the possibility of loading ExecuteListener configuration from an
XML configuration file. Settings is the right place for
ExecuteListeners.

Settings might be the right place to define which listeners to attach but is it the right place to keep the instances, too?

Why don't you create the listeners once in Configuration when setSettings() is called and then use this list?

Regards,

A. Digulla

[Lukas Eder]

Hi Aaron,

> If you look at the code, you'll see that it doesn't make sense. It would be
> simple to get rid of the ConnectionProvider since only Configuration uses
> it. But if I hadn't added it, there would be no chance for you to even look
> at the idea.

OK, I understand.

> Every application out there and every framework has their own GREAT IDEA(TM)
> how to get a connection from JDBC.

And you're adding one more :-)

> No one uses the JDBC API directly; they
> use Spring or Hibernate or Transaction managers or JNDI or whatnot.

I was under the impression that using JNDI to look up a JDBC
DataSource is the way to go in J2EE. And that using Spring API to look
up a DataSource is the way to go in Spring. On popular request, I
added the convenience of being able to use JDBC DataSources to jOOQ.
Sergey's examples on the user group show how simple it is to configure
a jOOQ Factory using Spring. I'm not sure why you imply that jOOQ has
its own "GREAT IDEA(TM)". java.sql.DataSource seems to me to be a
standard and well-understood way to handle what you call a
ConnectionProvider. Or, in your own words, instead of adding yet
another "GREAT IDEA(TM)", I prefer to go with the standard, which is
JDBC.

> Yes, it would be great if they simply gave you the connection. But they
> don't.

Who is "they"?

> Everyone tries to be smart. So my solution is to wrap all this
> b****** in a single place to prevent frameworks and applications from
> leaking into jOOQ.

Was it something I said? :-)

> If you want to add JNDI support, instead of adding yet another constructor
> to Factory and everything that extends it, you write a
> JndiConnectionProvider. If you have Sping, you can write a
> JdbcTemplateConnectionProvider or use the DefaultConnectionProvider. If you
> use Hibernate, ... you get the idea.

I get the idea, yes.

> As you can see, your approach is to add N*M constructors while my approach
> uses a single constructor and an interface which defines how jOOQ
> communicates with the outside world.

So far, I wasn't aware of the fact that the 5 Factory constructors in
jOOQ 2.3 and the 6 Factory constructors jOOQ 2.4 were a problem to any
user. Constructor overloading for convenience seems to be a quite
common practice to me. Example: java.util.HashMap (4 constructors)

> Having JDBC API compatibility is just an additional bonus.

With #1498 fixed, I can compile jOOQ with JDK 6 and 7

> OO imposes some limitations on us and Java even more so. Someone thought
> that interfaces are a great idea and now, we have to live with it. But is it
> really better to add a new set of constructors for every connection source?
>
> How did you plan to add JNDI support to jOOQ?

I won't add JNDI support to jOOQ

> You can create a configuration in the existing constructors and deprecate
> them. I didn't because I wanted to demonstrate the effect.

OK

>> I'm not sure why cloning of Configuration attributes should be done...?
>
> That depends on the life cycle of the configuration. If you pass a Oracle
> configuration to H2Factory, the dialect must be replaced. If you simply
> changed it without cloning, Oracle won't be happy later.

Good point. Moving the SQLDialect out of the Factory, into the
Configuration would mean that H2Factory will be somewhat obsolete. At
least its stateful part, the static methods providing access to
H2-specific functionality will remain.

> In that case, my change is correct but maybe it would be better to add a
> counter to the DataSourceConnectionProvider to make sure the connection is
> only closed after the last piece of code is done with it.

Hmm, I currently do not have a test case for that. But I'm assuming
that the current design is correct.

1. If jOOQ operates on a JDBC Connection, that connection must be
closed in client code
2. If jOOQ operates on a JDBC DataSource, then that DataSource is
wrapped in a DataSourceConnection, which pretends that it actually is
a Connection. Every time a statement execution is finished, the
delegate Connection, which was fetched from the wrapped DataSource is
closed. When another statement is executed on the same
DataSourceConnection, jOOQ retrieves a new delegate Connection from
the wrapped DataSource.

These facts are hidden from jOOQ's internals, so jOOQ doesn't notice
the difference between 2. and 1.

Cheers
Lukas

[Lukas Eder]

Hi Sergey

> 1. We can leave current approach and mark it as deprecated.
> Jooq does not know which classloader should be used to load ExecuteListener
> instance and how to configure it.
> We should be able to instantiate and configure ExecuteListener instance and
> after that inject it into the Factory.
> The same approach is used in JdbcTemplate class (Spring framework).
> We can inject both DataSource and SQLExceptionTranslator instances into
> JdbcTemplate.
> http://static.springsource.org/spring/docs/2.0.x/api/org/springframework/jdbc/support/JdbcAccessor.html#setExceptionTranslator%28org.springframework.jdbc.support.SQLExceptionTranslator%29

Yes, I understand that Spring has the advantage of being able to use
Spring for configuration. Without assuming any dependencies though,
jOOQ uses JAXB for loading generic XML configuration files. Thanks to
your recent changes to jOOQ's XJC code generation, JAXB artefacts and
Spring configurations are compatible.

However, moving ExecuteListeners out of Settings would mean that other
users could no longer use their XML files / JAXB, to configure jOOQ's
ExecuteListener settings. I am not happy with the fact that the
current solution actually loads classes. But I would like to keep the
ExecuteListener property in Settings, somehow. The currently ongoing
thread with Aaron Digulla also depicts that separation of concerns
between Factory and Configuration should be improved. Moving another
item to the Factory would be a step back in that evolution.

Another constraint that I would prefer not to be broken: The
ExecuteListener object lifecycle is the same as that of a query
execution. An ExecuteListener is allowed to contain state, which it
may share between events. E.g. a state can be produced at the
renderStart() event, and consumed at the renderEnd() event. Your
solution would imply that ExecuteListeners were allowed to live longer
than that, which introduces the risk of stale state between subsequent
executions of queries.

Anyone from the group, please participate. Do you see a solution
solving Sergey's classloading problem while preserving compatibility
as much as possible?

> 2. I like idea to use DataSourceConnection, DataSourcePreparedCall,
> DataSourcePreparedStatement etc. classes to close connection received from
> DataSource. It is pretty elegant and works fine. I suppose it's safe to
> remove FactoryProxy in 2.4.

Thanks for the feedback. FactoryProxy will be removed in jOOQ 3.0

> The only questions was why you decided to use wrappers instead of proxies
> (from java.lang.reflect package).

That is a nice hint. I hadn't explored that possibility. Specifically,
DataSourceConnection exposes getDataSource() to DataSourceStatement,
in order for the latter to detect whether the underlying Connection
needs to be closed. But I guess that could be done with proxies as
well.

I'll put this idea on the roadmap for future consideration: #1538
https://sourceforge.net/apps/trac/jooq/ticket/1538

Cheers
Lukas

2012/7/5 Sergey Epik <serge...@gmail.com>:

> Hello Lukas,
>
>
> On Thu, Jul 5, 2012 at 8:55 PM, Lukas Eder <lukas...@gmail.com> wrote:
>>
>> Hi Sergey,
>>
>> I've had a closer look to your suggested contribution. This would
>> break existing API where ExecuteListeners are registered in Settings,
>> with the possibility of loading ExecuteListener configuration from an
>> XML configuration file. Settings is the right place for
>> ExecuteListeners.
>>
>> Do you see any other solution that would remain compatible?
>
>

> 1. We can leave current approach and mark it as deprecated.
> Jooq does not know which classloader should be used to load ExecuteListener
> instance and how to configure it.
> We should be able to instantiate and configure ExecuteListener instance and
> after that inject it into the Factory.
> The same approach is used in JdbcTemplate class (Spring framework).
> We can inject both DataSource and SQLExceptionTranslator instances into
> JdbcTemplate.
> http://static.springsource.org/spring/docs/2.0.x/api/org/springframework/jdbc/support/JdbcAccessor.html#setExceptionTranslator%28org.springframework.jdbc.support.SQLExceptionTranslator%29
>
> 2. I like idea to use DataSourceConnection, DataSourcePreparedCall,
> DataSourcePreparedStatement etc. classes to close connection received from
> DataSource. It is pretty elegant and works fine. I suppose it's safe to
> remove FactoryProxy in 2.4.
>
> The only questions was why you decided to use wrappers instead of proxies
> (from java.lang.reflect package).
> It's more simple to define InvocationHadler that intercepts "close" method
> and does all stuff.
> We should not implement JDBC interfaces in this case and are more protected
> from JDBC changes.
> Here is example, how Spring framework creates Connection proxy and
> intercepts "close" method:
> http://www.docjar.com/html/api/org/springframework/jdbc/datasource/TransactionAwareDataSourceProxy.java.html
> Look at the method getTransactionAwareConnectionProxy.
>
> --
> Best regards,
> Sergey Epik

[digulla]

Am Freitag, 6. Juli 2012 17:33:38 UTC+2 schrieb Lukas Eder:

> Every application out there and every framework has their own GREAT IDEA(TM)
> how to get a connection from JDBC.

And you're adding one more :-)

Yeah, after reading that again, I see how it was easy to misunderstand.

Overloading is a code smell. If you look at how the API turned out, it's easy to see how it would be more simple to move all the overloading into Configuration and have one or two Factory constructors. I understand that the API evolved and wasn't designed. I'm missing "you're right" or "I'll fix that" or "How can I improve that?" from your side. The feedback from you felt like "I don't care - it's not a problem" which I didn't understand. It's such an obvious sore spot and so easy to solve ... Either there is something that I'm missing or you don't get what I'm talking about.

Next point, same topic: It might even be a good idea to drop support for the ${db}Factory (H2Factory, MysqlFactory, ...) constructors to communicate the fact that the application code must tell jOOQ when it connects to a new type of database - jOOQ doesn't try to be smart and figure that out by itself.

As it currently is, I can create an OracleDataSource and then pass that to H2Factory and it will work for most SQL queries. Only when I try to call DB specific code, it might break. That makes me nervous.

> No one uses the JDBC API directly; they
> use Spring or Hibernate or Transaction managers or JNDI or whatnot.

I was under the impression that using JNDI to look up a JDBC
DataSource is the way to go in J2EE.

Yes but every application/non-jOOQ framework will come up with another great idea how to wrap a JDBC connection and how to request/return it. That's why I came up with a simple interface to rule them all, so to say. Someone comes up with way #512? Write 5-10 lines of code and be done with it.

> As you can see, your approach is to add N*M constructors while my approach
> uses a single constructor and an interface which defines how jOOQ
> communicates with the outside world.

So far, I wasn't aware of the fact that the 5 Factory constructors in
jOOQ 2.3 and the 6 Factory constructors jOOQ 2.4 were a problem to any
user. Constructor overloading for convenience seems to be a quite
common practice to me. Example: java.util.HashMap (4 constructors)

True but it doesn't have 20 and it doesn't get a new one every time someone creates a new kind of Map.

Overloading is a tool; It has pros and cons. Like any other code, some cons disguise themselves as advantages until you find yourself on a tree branch on the wrong side of the saw.

 

>> I'm not sure why cloning of Configuration attributes should be done...?
>
> That depends on the life cycle of the configuration. If you pass a Oracle
> configuration to H2Factory, the dialect must be replaced. If you simply
> changed it without cloning, Oracle won't be happy later.

Good point. Moving the SQLDialect out of the Factory, into the
Configuration would mean that H2Factory will be somewhat obsolete. At
least its stateful part, the static methods providing access to
H2-specific functionality will remain.

I like the idea but that decision makes it ever more important to carefully design the life cycle of the configuration + when it will be cloned or not.

Since rest of jOOQ is so ... "obvious" (doesn't try to be smart/guessing what the consumers of jOOQ might intend), I tend towards that jOOQ should just make a copy when the dialect changes but make shallow-copy of the other fields.

That way, consumers can come up with their own strategy when to deep-copy the config and when not.

 

> In that case, my change is correct but maybe it would be better to add a
> counter to the DataSourceConnectionProvider to make sure the connection is
> only closed after the last piece of code is done with it.

Hmm, I currently do not have a test case for that. But I'm assuming
that the current design is correct.

1. If jOOQ operates on a JDBC Connection, that connection must be
closed in client code
2. If jOOQ operates on a JDBC DataSource, then that DataSource is
wrapped in a DataSourceConnection, which pretends that it actually is
a Connection. Every time a statement execution is finished, the
delegate Connection, which was fetched from the wrapped DataSource is
closed. When another statement is executed on the same
DataSourceConnection, jOOQ retrieves a new delegate Connection from
the wrapped DataSource.

This design means that I can't use DataSource when I want to execute several queries in one transaction. Is that what you want?

My design allows to move (part of) the transaction design into the ConnectionProvider. It works like this:

- I always write code which uses the ConnectionProvider

- The query building code doesn't care about transactions; it's part of the ConnectionProvider's API

- When I want to run several queries in a single transaction, I just need to tell the ConnectionProvider to keep the connection open, for example by using a wrapper or code like this:

new TransactionAwareConnectionProvider( ConnectionProvider parentProvider, TransactionAware codeToExceute ) {

    void run( Configuration config ) { ... }

}

Regards,

A. Digulla

[digulla]

Am Freitag, 6. Juli 2012 17:59:39 UTC+2 schrieb Lukas Eder:

However, moving ExecuteListeners out of Settings would mean that other
users could no longer use their XML files / JAXB, to configure jOOQ's
ExecuteListener settings.

Why not leave the static config in settings and use Configuration to keep the list of instances?

In my mental model, Settings is a value object which doesn't change - a perfect place to keep a list of class names.

Configuration is the dynamic part - what jOOQ should use right now. It uses Settings to know how to set up itself as necessary.

If the instance creating code is moved there, this would also allow OSGi people to override the default behavior.

Another constraint that I would prefer not to be broken: The
ExecuteListener object lifecycle is the same as that of a query
execution. An ExecuteListener is allowed to contain state, which it
may share between events. E.g. a state can be produced at the
renderStart() event, and consumed at the renderEnd() event. Your
solution would imply that ExecuteListeners were allowed to live longer
than that, which introduces the risk of stale state between subsequent
executions of queries.

This is again the question of the life cycle. Since jOOQ is an SQL framework, I'd like these instances to have the same life time as the transaction. I'm pretty sure it would be useful for ExecuteListeners to know when a transaction starts/ends.

That would be a perfect time to reset state, for example.

Regards,

A. Digulla

[Lukas Eder]

> I'm missing "you're right" or "I'll fix
> that" or "How can I improve that?" from your side. The feedback from you
> felt like "I don't care - it's not a problem" which I didn't understand.

I hope you're not taking things too personal here. I didn't mean to
make that impression. I can see how some things are problems. Please
don't confuse "I'm not sure if you are entirely right" with "I don't
care".

> It's such an obvious sore spot and so easy to solve ... Either there is
> something that I'm missing or you don't get what I'm talking about.

I don't think it's been so obvious. I keep track of requests on this
user group and eventually, the best design is created in collaboration
with users. Of course, any design may be challenged and I thank you
for that. But this "issue" didn't appear so far, so I still believe
there current API is not absolutely wrong, which is a precondition for
my saying "You're right".

I do get what you're talking about. In my opinion, however
javax.sql.DataSource is precisely that ConnectionProvider. Client code
may implement DataSource.getConnection() and Connection.close(). Then
they can inject any transactional behaviour they want into jOOQ (in
addition to supporting JDBC / J2EE standard ones), as jOOQ calls
DataSource.getConnection() and Connection.close() at the right
moments.

Your solution simply consists in changing names and adding the
convenience of dealing with a single (proprietary) interface instead
of two (standard ones):

- DataSource => ConnectionProvider
- DataSource.getConnection() => ConnectionProvider.getConnection()
- Connection.close() => ConnectionProvider.returnConnection()
- Connection.isClosed() => ConnectionProvider.isConnected()

> Next point, same topic: It might even be a good idea to drop support for the
> ${db}Factory (H2Factory, MysqlFactory, ...) constructors to communicate the
> fact that the application code must tell jOOQ when it connects to a new type
> of database - jOOQ doesn't try to be smart and figure that out by itself.

Yes, you're right :-)
Those factories are misleading.

> That way, consumers can come up with their own strategy when to deep-copy
> the config and when not.

What I'm most confused about: This discussion started off with JDK 7
(JDBC 4.1) incompatibilities. That was an actual issue, and I hope
jOOQ 2.4 had resolved that. Then it was about separation of concerns.
Then, about cloning Configurations. Maybe I just don't understand your
problem domain, while you already tell me solution proposals? And now
you expect me to say "You're right"? :-)

> This design means that I can't use DataSource when I want to execute several
> queries in one transaction. Is that what you want?

Either you handle your connection / transaction lifecycle yourself,
then you can pass jOOQ a JDBC Connection. Or you still handle your
transaction lifecycle yourself (e.g. using a
javax.transaction.UserTransaction) and let jOOQ handle the JDBC
Connection lifecycle by passing a DataSource. Am I missing something?

In any case, jOOQ will not handle the transaction.

> - I always write code which uses the ConnectionProvider
> - The query building code doesn't care about transactions; it's part of the
> ConnectionProvider's API
> - When I want to run several queries in a single transaction, I just need to
> tell the ConnectionProvider to keep the connection open, for example by
> using a wrapper or code like this:
>
> new TransactionAwareConnectionProvider( ConnectionProvider parentProvider,
> TransactionAware codeToExceute ) {
> void run( Configuration config ) { ... }
> }

Yes, that would be a nice design for client code. Instead of
implementing ConnectionProvider, you could implement DataSource (or
wrap it) and handle the transaction all the same. void
run(Configuration) would become void run(Factory) for now...

Cheers
Lukas

[Lukas Eder]

Hi Aaron,

(I'll reply to your other mail later...)

>> However, moving ExecuteListeners out of Settings would mean that other
>> users could no longer use their XML files / JAXB, to configure jOOQ's
>> ExecuteListener settings.
>
> Why not leave the static config in settings and use Configuration to keep
> the list of instances?
>
> In my mental model, Settings is a value object which doesn't change - a
> perfect place to keep a list of class names.
>
> Configuration is the dynamic part - what jOOQ should use right now. It uses
> Settings to know how to set up itself as necessary.

Today, the "default" Settings object is simply cloned into a "local"
Settings object, whose lifecycle is that of a Factory/Configuration.
So, in a way, this is already done, except for the fact that both the
"default" as the "local" copy operate on ExecuteListener Class
objects, not instances.

In other words, your idea is the same as Sergey's.

> If the instance creating code is moved there, this would also allow OSGi
> people to override the default behavior.

Yes, as a workaround, this would solve those problems. And it would
seem consistent, with the slight drawback, that no default (=
Settings) values for ExecuteListeners could be configured in such OSGi
environments.

On the other hand, maybe XML configuration through JAXB really isn't
the best way for Settings to work?

> This is again the question of the life cycle. Since jOOQ is an SQL
> framework, I'd like these instances to have the same life time as the
> transaction. I'm pretty sure it would be useful for ExecuteListeners to know
> when a transaction starts/ends.

Since the early days of jOOQ, I explicitly wanted to keep transaction
handling outside of jOOQ. How would you communicate transaction state
to jOOQ's internals in case you're using a
javax.transaction.UserTransaction? In that case, transaction handling
is decoupled from connection pooling in client code, i.e. a
transaction can span several dataSource.getConnection() /
connection.close() calls.

> That would be a perfect time to reset state, for example.

By "reset state", you mean creating new ExecuteListener instances?

[Lukas Eder]

> Your solution simply consists in changing names and adding the
> convenience of dealing with a single (proprietary) interface instead
> of two (standard ones):
>
> - DataSource => ConnectionProvider
> - DataSource.getConnection() => ConnectionProvider.getConnection()
> - Connection.close() => ConnectionProvider.returnConnection()
> - Connection.isClosed() => ConnectionProvider.isConnected()

To wrap these things up, for the next release 2.5.0, I'll analyse
whether an additional Factory(ConnectionProvider) constructor would be
a viable *complement* to what we have today. While I didn't agree that
such a solution should be a *replacement* (most users want to keep it
simple), I do acknowledge that it could add some value to some users.
Internally, jOOQ would implement two ConnectionProviders (one for
operating with a Connection and one for operating with a DataSource).
These two providers might be publicly exposed. These might obsolete
the existing DataSourceConnection and other classes that were
implemented recently. This will be tracked as #1549:

- https://sourceforge.net/apps/trac/jooq/ticket/1549

Overloading the Factory constructor once more is OK in the sense that
today, Factory IS-A Configuration. In jOOQ 3.0, this IS-A relation can
be resolved into a HAS-A relation. Various tickets indicate change to
the public API as of jOOQ 3.0:

- https://sourceforge.net/apps/trac/jooq/ticket/1192 (Review public
API type hierarchy and reduce complexity)
- https://sourceforge.net/apps/trac/jooq/ticket/1372 (Separate query
construction from query execution in Factory)
- https://sourceforge.net/apps/trac/jooq/ticket/1533 (Decouple Factory
from Configuration. Use composition instead of API inheritance)

Cheers
Lukas

[digulla]

Am Montag, 9. Juli 2012 18:22:09 UTC+2 schrieb Lukas Eder:

What I'm most confused about: This discussion started off with JDK 7
(JDBC 4.1) incompatibilities. That was an actual issue, and I hope
jOOQ 2.4 had resolved that. Then it was about separation of concerns.
Then, about cloning Configurations. Maybe I just don't understand your
problem domain, while you already tell me solution proposals? And now
you expect me to say "You're right"? :-)

These topics are loosely connected. I have a talent to pick at code and find the sore spots really quickly. When I start to pull, there is a ripple effect. It drives project owners insane but often, I'm right and the reason why it's not changed is because it's too much effort. Then things stay broken which drives me insane.

 

Yes, that would be a nice design for client code. Instead of
implementing ConnectionProvider, you could implement DataSource (or
wrap it) and handle the transaction all the same. void
run(Configuration) would become void run(Factory) for now...

Which leads back to where this whole discussion started: Client code can't implement DataSource because the interface changes with every version of JDBC and that means you must have a build system which can compile code differently per Java version. Only, there is no standard build system for this, so every project must invent their own wheel.

My solution: Create a wrapper instance that doesn't depend on the Java version and which accepts DataSource from the current runtime - no more dependency on the Java version.

Does my solution suck? Yes.

Does implementing JDBC interfaces suck more? Hell, yes.

Can we get Oracle to stop this? Not likely plus the past is already broken.

So my solution causes the least amount of pain and it even gives users of Java 1.4 a chance without an additional cost on your side.

Regards,

A. Digulla

[Lukas Eder]

> These topics are loosely connected. I have a talent to pick at code and find
> the sore spots really quickly. When I start to pull, there is a ripple
> effect. It drives project owners insane but often, I'm right and the reason
> why it's not changed is because it's too much effort. Then things stay
> broken which drives me insane.

It's not at all much effort. I'd implement your ConnectionProvider in
30mins. But it'll break central API, which is not acceptable between
minor releases.
I have committed to semantic versioning (thanks Vladislav!), so a
radical change is impossible before jOOQ 3.0. That'll keep you insane
for a bit :-)

Besides, there are worse problems in jOOQ, believe me. To name a few:

- Data type initialisation is a mess
(http://sourceforge.net/apps/trac/jooq/ticket/650)
- Generated class initialisation can lead to concurrency issues
(http://sourceforge.net/apps/trac/jooq/ticket/1543)
- The last remainders of "attachability" increase complexity
(http://sourceforge.net/apps/trac/jooq/ticket/1544)
- Some usage of generics is probably not optimal
(http://sourceforge.net/apps/trac/jooq/ticket/1374)
- Too many marker interfaces are hard to understand for newbies
(http://sourceforge.net/apps/trac/jooq/ticket/1192)
- I'm still undecided about TableField<R, T>, i.e. whether any field
should have an R type

> Does my solution suck? Yes.

... and ... as I said, it's now on the roadmap.

> So my solution causes the least amount of pain and it even gives users of
> Java 1.4 a chance without an additional cost on your side.

Good luck to them finding all of jOOQ's invocations of
String.contains() and String.isEmpty() and similar and patch those :-)

[Lukas Eder]

Hi Sergey,

> 1. We can leave current approach and mark it as deprecated.
> Jooq does not know which classloader should be used to load ExecuteListener
> instance and how to configure it.

I'm still evaluating options regarding this issue here. Options
proposed so far would have a major impact on the API strategy (meaning
of Configuration / Settings), so I'd like to avoid them in a minor
release. I've asked a question on Stack Overflow to see if there are
alternatives:
http://stackoverflow.com/questions/11483993/is-there-an-alternative-to-loading-a-class-with-class-forname

Cheers
Lukas

2012/7/5 Sergey Epik <serge...@gmail.com>:
> Hello Lukas,
>
>

> On Thu, Jul 5, 2012 at 8:55 PM, Lukas Eder <lukas...@gmail.com> wrote:
>>

>> Hi Sergey,
>>
>> I've had a closer look to your suggested contribution. This would
>> break existing API where ExecuteListeners are registered in Settings,
>> with the possibility of loading ExecuteListener configuration from an
>> XML configuration file. Settings is the right place for
>> ExecuteListeners.
>>
>> Do you see any other solution that would remain compatible?
>
>

[Lukas Eder]

> I'm still evaluating options regarding this issue here. Options
> proposed so far would have a major impact on the API strategy (meaning
> of Configuration / Settings), so I'd like to avoid them in a minor
> release. I've asked a question on Stack Overflow to see if there are
> alternatives:
> http://stackoverflow.com/questions/11483993/is-there-an-alternative-to-loading-a-class-with-class-forname

... and there is already an answer! Could you please check if this
will work for your set up?
http://stackoverflow.com/a/11484093/521799

My integration tests (using Eclipse classloaders) work fine with that

Cheers
Lukas

[Sergey Epik]

Hello Lukas,

Some notes regarding previous discussion. I not sure that ConnectionProvider is really necessary. We have DataSource interface and I suppose it's enough.

Moreover, configuration with Connection may be replaced with SingleConnectionDataSource, like this:
http://static.springsource.org/spring/docs/1.1.5/api/org/springframework/jdbc/datasource/SingleConnectionDataSource.html

There are several new methods in Factory: setAutoCommit, setTransactionIsolation etc. Not sure these methods can be used with DataSource connections (nobody cares about closing).

--

[Lukas Eder]

Hi Sergey

> This solution works.

I will integrate it into jOOQ for now. In the mean time, that
particular answer was deleted again on Stack Overflow. It seems that
using context class loaders is a sign of bad design (yet not as bad as
using Class.forName())

See the updated question here:
http://stackoverflow.com/questions/11483993/is-there-an-alternative-to-loading-a-class-with-class-forname

I will keep these things in mind when implementing jOOQ 3.0. So far, I
like your approach best, where ExecuteListener *instances* (not
*classes*) can be injected into a Configuration. With jOOQ 3.0's
separation of Configuration / Factory, things will get much cleaner.
I'll also consider Aaron Digulla's suggestion about having what we now
call "Settings" to be the default "Configuration" if not overridden
programmatically.

> Please pay attention that we should not keep classes in
> static field.
> I mean

>
> private static final Map<String, Class<?>> EXECUTE_LISTENERS = new
> ConcurrentHashMap<String, Class<?>>();
>

> in Util.
>
> If we "restart" osgi bundle with listener implementation, EXECUTE_LISTENERS
> will contain outdated classes.

I see. I guess that this cache causes more problems than it resolves.
The overhead from loading one class per listener per query should be
negligible, so I can safely remove this micro-optimisation.

The two issues will be resolved with #1572:
https://sourceforge.net/apps/trac/jooq/ticket/1572

- Usage of the context class loader
- Removal of the class cache

> I missed that ExecutionLister is created for each request (in this case more
> correct is to inject into "Configuration" some kind of
> ExecutionListenerFactory instance).

You're probably right. The current lifecycle may not be intuitive.
This will be obsolete in jOOQ 3.0, though, when you can inject
instances into "Configuration", rather than classes into "Settings".

> We use ExecutionListener as exception translator (it maps Oracle exceptions
> ORA-xxx to java application-specific exceptions).
> I see no reason why it should be instantiated and configured by jOOQ (and
> per request).

The only reason is the fact that at some point I've decided to allow
for XML configuration of "Settings" and thus, ExecuteListeners. Using
JAXB to load that XML file, it wasn't possible to actually load
instances. Hence, the current design.

> You can remove EXECUTE_LISTENERS and instantiate using
> Thread.currentThread().getContextClassLoader().loadClass,
> but I can not recommend this solution.

Nope. Let's live with it until jOOQ 3.0, though.

> Thank you for introducing ExecutionListener and runtime exceptions.

You're welcome!

[Lukas Eder]

>> You can remove EXECUTE_LISTENERS and instantiate using
>> Thread.currentThread().getContextClassLoader().loadClass,
>> but I can not recommend this solution.
>
> Nope. Let's live with it until jOOQ 3.0, though.

The change of behaviour for ExecuteListeners will be tracked as #1578:
https://sourceforge.net/apps/trac/jooq/ticket/1578

[Lukas Eder]

> Some notes regarding previous discussion. I not sure that ConnectionProvider
> is really necessary. We have DataSource interface and I suppose it's enough.

That was my impression. Aaron's arguments against using DataSource and
in favour of using a proprietary ConnectionProvider seem to have been
originating from frustration over frequent incompatible JDBC API
changes, and the fact that DataSource leaves Connection
handling/pooling/closing open to implementations. So for some users, a
ConnectionProvider might indeed be simpler to configure than a
DataSource

> Moreover, configuration with Connection may be replaced with
> SingleConnectionDataSource, like this:
> http://static.springsource.org/spring/docs/1.1.5/api/org/springframework/jdbc/datasource/SingleConnectionDataSource.html

Yes, I share that opinion with you. DataSource is just as good as
ConnectionProvider. For convenience, it might be simpler for many
users, to be able to operate on a Connection directly, though, without
having to wrap things in DataSources / ConnectionProviders.

> There are several new methods in Factory: setAutoCommit,
> setTransactionIsolation etc. Not sure these methods can be used with
> DataSource connections (nobody cares about closing).

No, they shouldn't be used with DataSource connections, but with
standalone connections instead, when you have control over the wrapped
Connection. I will add some info to the relevant Javadoc of these
methods. Thanks for pointing it out.

For now, I think jOOQ 2.x will be fine with what we have
(Connection-mode, DataSource-mode)

Cheers
Lukas