Bugzilla – Full Text Bug Listing
| Summary: | [Hibernate] Handle custom EDataType value conversion in HibernateStore when possible | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | SWT <chewbizz> |
| Component: | cdo.core | Assignee: | Martin Taal <mtaal> |
| Status: | CLOSED WONTFIX | QA Contact: | Eike Stepper <stepper> |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | mtaal, stephane.fournier, stepper |
| Version: | 4.8 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
SWT
Created attachment 192731 [details] Modified "company" Ecore model used to reproduce the hibernate spatial case This file replaces the company.ecore file in plugin "org.eclipse.emf.cdo.examples.company" (R3_0_maintenance). You'll also need to add the JTS library which contains the custom type "com.vividsolutions.jts.geom.Geometry" : http://www.vividsolutions.com/jts/bin/jts-1.8.0.zip Created attachment 192732 [details]
Modified QuickStartTest used to reproduce the hibernate spatial case
This file replaces the org.eclipse.emf.cdo.examples.hibernate.client.QuickStartTest class in plugin "org.eclipse.emf.cdo.examples.hibernate.client" (R3_0_maintenance).
As does the attachement #192731 (company.ecore), it requires the JTS library.
Created attachment 192734 [details] Modified cdo-server.xml file used to reproduce the hibernate spatial case This file replaces the config/cdo-server.xml file in plugin "org.eclipse.emf.cdo.examples.hibernate.server" (R3_0_maintenance). You will need to install a PostgreSQL database, and the PostGIS extension : http://www.postgresql.org/download/ http://postgis.refractions.net/download/ And you'll also need to add some libraries to the server plugin : * JTS (jts-1.8.jar) : http://www.vividsolutions.com/jts/bin/jts-1.8.0.zip * Hibernate spatial and the PostGIS provider (hibernate-spatial-1.0.jar and hibernate-spatial-postgis-1.0.jar) : http://www.hibernatespatial.org/download.html * Postgis (postgis-1.5.0.jar) : http://postgis.refractions.net/download/ * javassist-3.11.0.GA.jar * slf4j-api-1.5.8.jar * slf4j-log4j12-1.6.1.jar * log4j-1.2.16.jar NOTE : All this is for PostgreSQL/PostGIS, but this should also work with any other hibernate spatial enabled database such as MySQL... You'll have to install the database and the corresponding spatial extension, and use the corresponding hibernate spatial provider JAR (and change the hibernate dialect accordingly). Patch proposals for commit operation : *** Current CDO "commit flow" : Client-side Custom Type values in EObjects --- pre-commit operations (I didn't check where the conversion happens) ---> Client-side String-for-transport values --- COMMIT TO SERVER ---> Server-side String-for-transport values in CDORevisions --- IStoreAccessor.write() ---> Server-side String-for-transport values in database rows (or in memory for MemStore, etc.) *** Patch proposal for CDO Hibernate Store : Client-side Custom Type values --- pre-commit operations ---> Client-side String-for-transport values --- COMMIT TO SERVER ---> Server-side String-for-transport values --- HibernateStoreAccessor.write() ---> Server-side database rows storing User Type values Where HibernateStoreAccessor.write() does : 1) Convert String-for-transport values to Custom Type values if the type's EFactory is available on the server side. 2) Store Custom Type values to DB using the corresponding UserType. *** Another proposal for CDO itself (instead of patching Hibernate Store) : Client-side Custom Type values --- pre-commit operations ---> Client-side String-for-transport values --- COMMIT TO SERVER ---> Server-side String-for-transport values --- TransactionCommitContext.adjustForCommit() ---> Server-side Custom Type values --- IStoreAccessor.write() ---> Server-side database rows storing values in a form specified by the IStoreAccessor. This latter proposal modifies CDO and not CDO Hibernate Store. Patching this way will allow Hibernate to work without this issue and without any modification. However, this is not perfect because IStoreAccessor will have to convert Custom Type values to something that can be stored. And in most cases, the conversion will consist in transforming back to String values. On the other hand, this will allow CDO to store values in DB in another form than the "String-for-transport" one, i.e. a "String-for-storage" representation. And for the sake of optimization, an IStoreAccessor could tell for each Custom Type if this "double conversion" should be performed or avoided. (If it is avoided, we fall back to the current behaviour...) Note that these proposals only address the "writing in store" part of the issue. I'll also have to dig into the "store reading" part... Can you please give me your opinion about this ? (And tell me if I made some mistakes ?) In my opinion, the second proposal is better, but then this bug may have to be relocated to "cdo.core", and the patch may break API compatibilty if one ore more IStoreAccessor methods must be added. Starting with your feedback, I will hopefully be able to provide an initial patch for you (if you allow me to do it). As Martin explained in the corresponding newsgroup thread, CDORevisionData and CDORevisionDelta contain String values for custom EDataTypes by design. The conversion on the *client* side takes place in CDOStoreImpl. We can certainly not accept patches that are trying to change this essential design in any way. AFAIK Martin asked for this bugzilla in order to convert the Strings back to their EDataType.instanceClass equivalent as part of Hibernate's mapping procedure, possibly optional, configurable, etc. (In reply to comment #5) > As Martin explained in the corresponding newsgroup thread, CDORevisionData and > CDORevisionDelta contain String values for custom EDataTypes by design. The > conversion on the *client* side takes place in CDOStoreImpl. We can certainly > not accept patches that are trying to change this essential design in any way. I understand that values are stored as String on the client side. Maybe I'm missing something but the proposed patches don't change the way CDO clients store values... It's only about converting these string values *on the server side*, either before they are sent to the IStoreAccessor, or by the IStoreAccessor itself. > AFAIK Martin asked for this bugzilla in order to convert the Strings back to > their EDataType.instanceClass equivalent as part of Hibernate's mapping > procedure, possibly optional, configurable, etc. Yes, and in order to implement it, I'd like to know if the HibernateStoreAccessor.write() method is a good place. And when searching for this "good place", I was wondering if the problem really is hibernate-specific. That is to say, CDORevision contents could be replaced by converted values (EDataType.instanceClass for custom types) at the CDO server core level. In the case of a commit operation, it would be done in TransactionCommitContext.write(), before calling IStoreAccessor.write() so that the IStoreAccessor doesn't work with String but with custom types directly. Anyway this is beyond the scope of this bugzilla, and I will stick with the straightforward patch of HibernateStoreAccessor. Yes, I think these are good starting points: HibernateStoreAccessor.readRevision() HibernateStoreAccessor.readRevisionByVersion() HibernateStoreAccessor.doWrite() Martin? I have a little question about how to implement the commit part of the patch : The method "HibernateStoreAccessor.write(InternalCommitContext, OMMonitor)" is called by TransactionCommitContext, passing "this" as first argument. CDORevisions are stored in this InternalCommitContext. My question is : Is it safe to modify values contained in CDORevisions inside this write method ? That is to say, is it possible that the TransactionCommitContext uses the CDORevisions again after the write operation, expecting String values inside these revisions ? This would be more efficient to modify the revisions directly because in the other case, all CDORevision objects in the context would have to be copied before changing the values... Please excuse me for bothering you with my silly questions, but I'm not a CDO developper, so I still need to learn more about the internals of CDO so that my patch doesn't have to be completely re-written... I hope I'll soon be able to send a first version of this patch :) (In reply to comment #8) > My question is : > Is it safe to modify values contained in CDORevisions inside this write method > ? That's definitely not allowed. We even plan to add code that prevents revisions from being changed at the wrong time. See bug 341081. > That is to say, is it possible that the TransactionCommitContext uses the > CDORevisions again after the write operation, expecting String values inside > these revisions ? If the commit operation was successful these revisions get globally cached in TransactionCommitContext.updateInfraStructure(). > This would be more efficient to modify the revisions directly because in the > other case, all CDORevision objects in the context would have to be copied > before changing the values... I'm not an expert for the design of the HibernateStore but I have the feeeling that the revisions should not be touched at all, also not copies of them. I recall that there are feature-specific "property getters/setters" that control the data flow between revisions and Hibernate/DB tables. I would try to do the conversion somewhere there on the fly. > Please excuse me for bothering you with my silly questions, but I'm not a CDO > developper, so I still need to learn more about the internals of CDO so that my > patch doesn't have to be completely re-written... > I hope I'll soon be able to send a first version of this patch :) No need to excuse! Noone starts as an expert anywhere and we're always happy to see engagement. IIRC Martin told me that he's with a customer for some time now, so bear with him if he can't answer Hibernate questions as quickly as usual. > I'm not an expert for the design of the HibernateStore but I have the feeeling > that the revisions should not be touched at all, also not copies of them. I > recall that there are feature-specific "property getters/setters" that control > the data flow between revisions and Hibernate/DB tables. I would try to do the > conversion somewhere there on the fly. Ok, I see. You're talking about CDOPropertyHandler classes. Thank you for pointing this out :) > No need to excuse! Noone starts as an expert anywhere and we're always happy to > see engagement. IIRC Martin told me that he's with a customer for some time > now, so bear with him if he can't answer Hibernate questions as quickly as > usual. No problem, I understand. I also have work that prevents me from spending a lot of time on CDO... But there's no hurry :) I now have a working patch for single-valued EAttributes using custom EDataType! The code is still a bit messy and there is not configuration setting indicating if value conversion should be performed... Even if I do not have much time for this, I hope I'll be able to finish by the end of next week. After some investigation, modifications are most likely to affect the following classes : - CDOPropertyGetter - CDOManyAttributeGetter - CDOPropertySetter - CDOManyAttributeSetter - HibernateMoveableListWrapper (?) - WrappedHibernateList (?) But one last question remains : How should this conversion be configured by users ? At first glance, I would suggest a global hibernate configuration property named "instanceTypeConversion" (please suggest a better name!) with 4 possible values : - ALWAYS : always try to perform conversion (this is the most useless value, but it should be available IMO). - NEVER : never perform any conversion for custom types (when the user knows that he doesn't use any custom type that requires conversion) => slightly improves perfs. - USER_TYPE : perform conversion only on custom types for which a JPA annotation associates a hibernate UserType to the EDataType. - ANNOTATED : perform conversion only on custom types for which a specific annotation (to be defined) is set on the corresponding EDataType. Note that when this setting tells that a custom type must be converted and the corresponding EPackage is not available on the server side, an exception can be thrown. I think the USER_TYPE case should be the default. Can you please provide some feedback about this before I start implementing it ? Thanks in advance :) > - USER_TYPE : perform conversion only on custom types for which a JPA
> annotation associates a hibernate UserType to the EDataType.
Edit :
- USER_TYPE : perform conversion only on custom types for which the hibernate mapping associates a hibernate UserType to the EDataType.
(because the hibernate mapping may be built from a file rather than from JPA annotations in the model).
Created attachment 193807 [details]
Patch proposal (incomplete but fully working)
Two things have been introduced in this patch :
- A new hibernate property called "instanceTypeConversion" (maybe should it be "hibernate.instanceTypeConversion" ?), which values can be :
"always", "never", "userType", "annotated", as described in previous notes.
- When instanceTypeConversion=="annotated", EDataTypes on which conversion must be performed must contain an EAnnotation with source=="instance_type_conversion".
This patch is "incomplete" because the USER_TYPE configuration setting does not work yet.
There is a TODO task tag for it...
(There are also two other minor TODO task tags left.)
Don't hesitate to ask for more info :)
Hi, I have been looking at your patch and it looks good to me. The 'problem' is that the patch is > 250 lines so it has to go through a legal review by the Eclipse legal department. I will keep you posted if I know more about the progress. Thanks for your effort! gr. Martin Hi, For the legal process I will need your: name organization email address (I guess I can use: swt@magellium.fr) Can you provide this? gr. Martin > Can you provide this? I just sent you a email (to mtaal@elver.org) with the required info. Thanks, I entered this cq: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5172 to get the patch legally approved. gr. Martin This is not yet commited, right? (In reply to comment #18) > Thanks, I entered this cq: > https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5172 > > to get the patch legally approved. > > gr. Martin It takes a lot of time, doesn't it ? Is there any problems ? (I don't have IPZilla access...) TIA (In reply to comment #20) > It takes a lot of time, doesn't it ? > Is there any problems ? (I don't have IPZilla access...) Not that we know of. The weeks preceding the yearly coordinated release is just a busy time for the IP team. There has been a reaction from the legal department: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5172#c5 Stephane, they request you to confirm a few things, can you check it out? gr. Martin Stephane, please say this: 1. I authored 100% of the contribution 2. I have the rights to contribute the content to Eclipse 3. I contribute the content under the Eclipse Public License Thx ;-) Yes of course : I authored 100% of the contribution. I have the rights to contribute the content to Eclipse. I contribute the content under the Eclipse Public License. Stéphane Wasserhardt Thanks, I have copied your reply into the CQ for the legal department. gr. Martin Moving all open enhancement requests to 4.1 Still no news from the legal department ? It would be great if this patch can be included the 4.1 release... The legal dept. has approved the CQ in the meantime. I guess we can proceed ;-) Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Revisiting this topic after a long time.. I apologize for this, it has been too long for various reasons.. The CDO Hibernate Store has recently been improved a lot and now all relevant test cases pass. For this topic. In hinsight, why not implement a UserType which can handle getting Strings from the CDO layer, converts the string to the intended custom EDataType instance and then persists this custom type in the database using one or more columns. So the UserType should not expect a custom instance but instead a String and take care of that conversion itself. This would work out of the box in CDO/Teneo afaics. gr. Martin Yes, it's been a long time for me too... I hope to work with CDO again next year, but I was assigned to projects not using CDO this year... Your solution is effectively the best when the user can implement its own user type and handle Strings. That's why I exposed the problem using the "hibernate spatial case", where the "GeometryUserType" Hibernate UserType is not the user's one, but one provided by others (vividsolutions in this case). This GeometryUserType stores Geometry objects, not String objects. As clients of the hibernate spatial library, we cannot change the GeometryUserType behaviour. This Geometry scenario is a common scenario when using database extensions. In my case I used a PostGres DB with the PostGIS extension which enables working with Geometry objects, offers dedicated DB functions, (spatial) indexing, etc. The same extension exists for Oracle, it is "Oracle Spatial". Both Oracle Spatial and PostGIS can be handled through Hibernate using the "Hibernate Spatial" extension, which contains the GeometryUserType, which allows clients to work directly with the Geometry objects that can be stored in a PostGIS or Oracle Spatial DB. I hope this clarifies a bit. That's why I think this patch is still useful. It may be simplified by removing useless configuration cases where the user could create it's own user type which maps to String objects. Hi, Okay, I think the following could be a solution (inline with your patch): Add an annotation to an eattribute/edatatype which somehows tells the hibernate store that hibernate expects the actual type (and not a string), and then use this annotation to do the type conversion. The type conversion should be done for both the single occurrence eattribute as well as the many occurrence eattribute. Still there is an easy workaround for now, create your own user type extending the standard usertype (GeometryUserType for example) and do the string-instance conversion in that usertype. This is little work imo. gr. Martin You're right. And what you suggest about a specific annotation is what I called the "ANNOTATED" case. It would be sufficient to manage only this case. This would lead to a much simpler patch. I would be pleased to spend some time on it, but I think I am not able to do it before next year :( Hi, It is no problem to wait :-), I will keep the bugzilla open, if I get to it earlier you will get a notification for it. Thanks! gr. Martin Moving all outstanding enhancements to 4.3 Moving all open enhancement requests to 4.4 Moving all open enhancement requests to 4.4 Moving all open bugzillas to 4.5. Moving all unaddressed bugzillas to 4.6. Moving all open bugs to 4.7 Moving all unresolved issues to version 4.8- Hibernate support has been deprecated, see bug 552307. |