Community
Participate
Working Groups
Build Identifier: Referring to the Eclipse forum thread in [1], the SQL state used for unique index/primary key violation exceptions was changed from 23001 to 23505 in H2, so this will require overriding the isDuplicateKeyException(SQLException) in H2Adapter if CDO (and, as I understand, Orbit) upgrades to a more recent version. See also a related Spring bug report in [2]. [1] http://www.eclipse.org/forums/index.php/m/659583/ [2] https://jira.springsource.org/browse/SPR-8235 Reproducible: Always
Created attachment 210102 [details] Proposed fix
Hi András! Thanks for the poposal. I think instead of using a hard-coded String, we could use: org.h2.constant.ErrorCode.DUPLICATE_KEY_1 This way, if the value changes, we don't need to change our code. You should also know that we are still using 1.1.117, I believe, but my proposal would ease the integration of different versions.
Oh, I was wrong, apparently version ranges were changed, so I guess we are supporting that version now ;) The code I propose would look like this: @Override public boolean isDuplicateKeyException(SQLException ex) { return String.valueOf(org.h2.constant.ErrorCode.DUPLICATE_KEY_1).equals(ex.getSQLState()); } We would also require to import package org.h2.constant
Created attachment 211792 [details] Improved fix using H2 API constants instead of hardcoded Strings
Thanks, Victor, for pointing out the existence of that constant class! I haven't seen it before. Unfortunately it looks like the bundles available on the official H2 site do not export org.h2.constant [1]; with regards to the Export-Package section, the contents of the manifest in trunk is consistent with what I'm seeing in a built bundle, version 1.3.160. [1] http://code.google.com/p/h2database/source/browse/trunk/h2/src/main/META-INF/MANIFEST.MF
Ouch! I believe they didn't use to provide OSGi bundles, so the ones we use (probably from springsource or self-made) do have that package exported. Thats bad news :( Is there a reason why the "constants" package is not exported? is it considered internal stuff? Any chance you could ask them? In such case, we can only go back to constant, but chances are it gets broken again in the future.
H2 Developer here: I think you are overcomplicating the solution. Just do: @Override public boolean isDuplicateKeyException(SQLException ex) { return "23001".equals(ex.getSQLState()) || "23505".equals(ex.getSQLState()); } Thomas Mueller (H2 Project Lead) doesn't change these constants often, and tries to track the relevant SQL standards, so the above code should be safe for quite a while. And I assume you have a unit test to catch any future problems.
Hi Noel, thanks for your feedback. See some comments below > Thomas Mueller (H2 Project Lead) doesn't change these constants often, and > tries to track the relevant SQL standards, so the above code should be safe for > quite a while. I can't seem to understand whats overcomplicated on "saving our backs" for future breaks. What is indeed complicated is to have fragile hardcoded constants around our code. Your proposal sounds like 'short-term gain, long term pain'. I don't know Mr Thomas, neither the reasons the code changed, but breaking it once seems enough reason not to take the same approach again, even if you guarantee it won't happen "in quite a while". > And I assume you have a unit test to catch any future problems. I'm not really a RDBMS expert, but common sense tells me standard constants does not change unless the standard changes itself. Granted the fact it may change, wouldn't it be safer to refer to a constant in the H2 API? Are you suggesting I should create test-cases for the whole Java SQL API, in case the underlying implementation changes? The thing is, not having that package exported will force us to adopt the hardcoded approach, so we don't have much alternatives here anyway.
Re: the other stuff - do whatever floats your boat. Re: the unit tests, I'm suggesting that you have a unit test that looks like: ... create a table ... try { .. insert duplicate key ... assertExceptionShouldHaveBeenThrown(); } catch (SQLException ex) { assertTrue(isDuplicateKeyException(ex)); }
Noel, the initial question was to ask if org.h2.constant.ErrorCode was meant to be internal, not used by clients. Is that the case? I kinda looks like well documented API. What's the reason not to export it? Any reason why it could be discouraged? Cheers, Víctor
As an aside, my understanding is that certain values of the first two digits of SQL states (the class part) are standardized, while the last three of them may be implementation-defined. Class 23 stands for integrity constraint violations, which includes (possibly among others) duplicate primary key/unique key issues as well as foreign key violations. I'm not aware of CDO specifying any foreign key or check constraints on tables it uses, so it might be OK to inspect only the class part and/or check if the thrown exception is an instance of java.sql.SQLIntegrityConstraintViolationException to cover a fairly broad range of database implementations (MYSQLAdapter checks for code 23000, for example). I might be mistaken on the above, however, so feel free to correct. It also might be important to note that currently only org.eclipse.emf.cdo.server.internal.db.mapping.horizontal.ObjectTypeTable.putObjectType(IDBStoreAccessor, long, CDOID, EClass) is calling this method, or at least that's what Eclipse's call hierarchy view wants to show me at the moment. :)
András, some inlined comments: (In reply to comment #11) > As an aside, my understanding is that certain values of the first two digits of > SQL states (the class part) are standardized, while the last three of them may > be implementation-defined. Class 23 stands for integrity constraint violations, > which includes (possibly among others) duplicate primary key/unique key issues > as well as foreign key violations. I'm not aware of CDO specifying any foreign > key or check constraints on tables it uses, so it might be OK to inspect only > the class part and/or check if the thrown exception is an instance of > java.sql.SQLIntegrityConstraintViolationException to cover a fairly broad range > of database implementations (MYSQLAdapter checks for code 23000, for example). > I might be mistaken on the above, however, so feel free to correct. Thats interesting, but not sure about the foreing key thing, maybe Stefan could tell. Is java.sql.SQLIntegrityConstraintViolationException JRE 1.6? In such case we wouldn't be able to use it, CDO needs to support Java 1.5 :(
Thanks for the compliment about our documentation :-) You're welcome to use the ErrorCode class, but it's not meant to be part of the public API of H2.
Noel, (In reply to comment #13) > Thanks for the compliment about our documentation :-) > > You're welcome to use the ErrorCode class, but it's not meant to be part of the > public API of H2. Ok, the I'll understand that as an "its internal" ;) Any chances for an enhacement request? What about exporting, but marked as "x-internal"? not sure how its interpreted out there, but in Eclipse it gives you a warning indicating "you shouldn't be using it, do it at your own risk".
(In reply to comment #12) > Is java.sql.SQLIntegrityConstraintViolationException JRE 1.6? In such case we > wouldn't be able to use it, CDO needs to support Java 1.5 :( Correct, it was added in 1.6.
(In reply to comment #14) > Noel, > > Any chances for an enhacement request? What about exporting, but marked as > "x-internal"? not sure how its interpreted out there, but in Eclipse it gives > you a warning indicating "you shouldn't be using it, do it at your own risk". Sounds fine to me, but I have no idea who controls our OSGi bundling stuff. Didn't even know it had been done :-)
Hey Guys, What CDO stream is this supposed to be in? It strikes me that these error constants are part of the public contract of H2, just not explicitely in Java. Hard for me to understand why. The x-internal approach is no real solution because, from a framework's perspective, x-internal is to be considered the same as not exported at all. At runtime it depends on the OSGi resolver mode (i.e., "strict") whether the package's content is visible or not. Nothing an application or a framework can control.
Eike, > What CDO stream is this supposed to be in? András should know, but I assume is 4.1. > The x-internal approach is no real solution because, from a framework's > perspective, x-internal is to be considered the same as not exported at all. At > runtime it depends on the OSGi resolver mode (i.e., "strict") whether the > package's content is visible or not. Nothing an application or a framework can > control. Ah, didn't know the strict thing. The thing would be to request making those public, but Noel suggests it's internal stuff not meant to be public.
(In reply to comment #18) > The thing would be to request making those public, Yeah. Or if the H2 team rejects this, make sure that Orbit's version of H2 does not expose more than the original set of packages (separate bugzilla!). > but Noel suggests it's internal stuff not meant to be public. I'm still interested in the reasoning of this suggestion ;-)
(In reply to comment #18) > Eike, > > > What CDO stream is this supposed to be in? > > András should know, but I assume is 4.1. Initially I assumed this request would depend on the time the H2 bundle in Orbit was updated to a more recent version, so I haven't set that field.
Eike, any ideas on how to tackle this bug?
Even if the H2 team would open up this handy interface, we'd depend on the newest H2 versions. I suggest we follow the advice in comment 7.
The mentioned Spring bug was also fixed by adding both keys to their corresponding configuration file, see [1]. [1] http://svnsearch.org/svnsearch/repos/SPRINGFRAMEWORK/search?p=/trunk/org.springframework.jdbc/src/main/resources/org/springframework/jdbc/support/sql-error-codes.xml&rev=5022&
(In reply to comment #23) > The mentioned Spring bug was also fixed by adding both keys to their > corresponding configuration file, see [1]. Hmm, this link leads to a 10 pages search result.
Sorry about that, I'll link to the most recent state of that configuration file instead [1]. The thing worth noting is that the backing bean for that file maps multiple SQL states to the same error condition. [1] https://github.com/SpringSource/spring-framework/blob/master/spring-jdbc/src/main/resources/org/springframework/jdbc/support/sql-error-codes.xml#L68
I'm going for the two String checks...
@Override public boolean isDuplicateKeyException(SQLException ex) { String sqlState = ex.getSQLState(); return "23001".equals(sqlState) || "23505".equals(sqlState); }
commit 81943e65d274673584283bb75f74c22950aec856
Closing.
