bpangburn / swingset

SwingSet is an open source Java toolkit containing data-aware replacements for many of the standard Java Swing components.
3 stars 2 forks source link

Add an option for how the demos get a RowSet #173

Closed errael closed 9 months ago

errael commented 11 months ago

This PR allows jdk standard JdbRowSet usage in the Demos. The idea is to update the commit/PR so that the one line change is made to each example.

Note that if DemoUtil.USE_NQADMIN_ROWSET is set to true, there is no change in behavior.

I'd like to track acquire/release of connections to investigate possible leakage of connections. I'm not sure that's even a real thing or if there might be a way to track this somewhere.

The PR could be merged, which is more convenient for me. But at least I'll use this commit locally, and rebase to where I'm working (grid/format/...) as needed.

errael commented 11 months ago

I'm OK with this approach. Looks like you still need to update the other example files to use DemoUtil.getNewRowSet()?

Right, 2nd sentence of the intro post.

I guess it doesn't matter much to me if USE_NQADMIN_ROWSET is false by default for the demo.

I think true is the best default; keep things the same.

I think this approach will end up creating a database connection for every screen vs. sharing one.

That's part of what I want to explore. I'd like to get it to the point where it requires no additional resources.

I was wondering if a sub-directory of demo, maybe support, makes sense.

bpangburn commented 11 months ago

That's part of what I want to explore. I'd like to get it to the point where it requires no additional resources.

That would be great and then I'd be open to eventually doing away with the old way.

I was wondering if a sub-directory of demo, maybe support, makes sense.

If you think that makes it a little cleaner, go for it!

errael commented 11 months ago

Things seem to work for CachedRowSet. I've discovered that they don't handle BLOB's; guess that makes sense, so the images are not displayed in Test Base Components.

This PR should not affect using a JdbcRowSet (of either origin). The only changes to mainline SwingSet should be conditioned on working with CachedRowSet. Perhaps the most interesting thing in this PR is DataSourceSharedConnection in Demo; this uses the JdbcRowSet from the JDK, and they all share a connection as requested.

bpangburn commented 10 months ago

@errael Is there anything here grid specific? I feel like we would want to merge this into 4.0.13-SNAPSHOT, but maybe there is a good reason not to. Not too worried about changes to the demo, but I want to spend some more time on the changes to SwingSet to confirm they are 'conditioned on working with CachedRowSet' as you mention.

bpangburn commented 9 months ago

I've had a chance to test this a bit. Everything works fine if I force Java 11 in the POMs. If I leave Java 8 in the POMs the the POOL_CACHED and SHARE_JDBC options throw: java.sql.SQLException: (JNDI) Unable to connect

Not sure if this is one of the reasons you wanted to move to Java 11.

errael commented 9 months ago

I've had a chance to test this a bit. Everything works fine if I force Java 11 in the POMs. If I leave Java 8 in the POMs the the POOL_CACHED and SHARE_JDBC options throw: java.sql.SQLException: (JNDI) Unable to connect

Not sure if this is one of the reasons you wanted to move to Java 11.

That's a new one for me. An SQLException? My guess is that has something to do with using the DataSource stuff, which uses JNDI to lookup the DataSource by name.

There's probably something about the trivial JNDI Context I put together that's not happy.

bpangburn commented 9 months ago

Based on the comment below from 2023-12-29, I was under the impression you wanted to stick with NQADMIN rowset in the demo by default. I realize that variable no longer exists because you added an ENUM. If we're going to jump to 11 now OR if there is a straightforward fix for the JNDI SQL Exception then I guess I still don't care what is used for the default. If we want to support 8 and there's not a straightforward fix for JNDI, I don't want the demo default to throw exceptions.

I'm OK with this approach. Looks like you still need to update the other example files to use DemoUtil.getNewRowSet()?

Right, 2nd sentence of the intro post.

I guess it doesn't matter much to me if USE_NQADMIN_ROWSET is false by default for the demo.

I think true is the best default; keep things the same.

I think this approach will end up creating a database connection for every screen vs. sharing one.

That's part of what I want to explore. I'd like to get it to the point where it requires no additional resources.

I was wondering if a sub-directory of demo, maybe support, makes sense.

errael commented 9 months ago

Actually, I probably thought that you wanted to stick with it:) Looking at the comment, it's discussing a boolean; it's probably referring to some early iteration of the code. There's three choices now.

As you see from the code, it's easy to go either way.

BTW, no matter which of the three choices, there's never more than one active connection (IIRC).

errael commented 9 months ago

I just noticed that your last comment mentions the JDK issue. Since DataSource has been around since jdk-4, there should be a way to get it to work with jdk-8.

I thought I'd seen mail from you that JDK-11 was OK, it's not worth rooting through mail since it's an issue. I'm in the middle of stuff right now; if you want to keep moving forward, you can remove the items that are causing problems from the combobox.

bpangburn commented 9 months ago

I just noticed that your last comment mentions the JDK issue. Since DataSource has been around since jdk-4, there should be a way to get it to work with jdk-8.

I thought I'd seen mail from you that JDK-11 was OK, it's not worth rooting through mail since it's an issue. I'm in the middle of stuff right now; if you want to keep moving forward, you can remove the items that are causing problems from the combobox.

I'm OK going to 11 if needed. That alone might be enough of a reason to make the next version 4.1.0.

No rush here. I have plenty of other PRs to review. I just wanted to mention the issue with 1.8 in case it's an easy fix.

errael commented 9 months ago

Based on the comment below from 2023-12-29, I was under the impression you wanted to stick with NQADMIN rowset in the demo by default. I realize that variable no longer exists because you added an ENUM. If we're going to jump to 11 now OR if there is a straightforward fix for the JNDI SQL Exception then I guess I still don't care what is used for the default. If we want to support 8 and there's not a straightforward fix for JNDI, I don't want the demo default to throw exceptions.

I'm OK with this approach. Looks like you still need to update the other example files to use DemoUtil.getNewRowSet()?

Right, 2nd sentence of the intro post.

I guess it doesn't matter much to me if USE_NQADMIN_ROWSET is false by default for the demo.

I think true is the best default; keep things the same.

I think I remember. At this stage in development the two choices were

  1. JdbcRowSet (as nqadmin's JdbcRowSetImpl)
  2. CachedRowSet

So when I said "keep things the same" I meant use a JdbcRowSet by default.

Later there are two ways to get a JdbcRowSet. I chose the, more or less, "standard" way. (with the trick to get a magic DataSourceName that provides the single connection).

Of course I could easily be remembering this incorrectly; and I'm undoubtedly slanting things in a particular way.

errael commented 9 months ago

If I leave Java 8 in the POMs the the POOL_CACHED and SHARE_JDBC options throw: java.sql.SQLException: (JNDI) Unable to connect

Trivial fix (less trivial to find).

It's a good change to make in general.

bpangburn commented 9 months ago

Looks like just one easy conflict to merge. I think we want the line from WhichRowSet: transient private SSComponentInterface ssComponent = null;

errael commented 9 months ago

Looks like just one easy conflict to merge. I think we want the line from WhichRowSet: transient private SSComponentInterface ssComponent = null;

I think we want the one that's final. That was the whole idea, can't be changed. (at least that was one of the PRs.)

errael commented 9 months ago

I've never used any github's merge/conflict stuff. AFAICT, the line with "final" was not picked.