Comment 1 Daniel Ford 2010-05-01 16:27:13 EDT

Created attachment 166720 [details]
Source to recreate the exception.

Comment 2 Caspar D. 2010-05-04 04:27:02 EDT

The bundle com.ibm.almaden.mdht.cdo.example1 requires a
bundle org.openhealthtools.mdht.uml.cda, but the latter
isn't included in the attached ZIP file; so, I can't
build/run this test code.

(Furthermore, it would also be nice if test code didn't
require additional depedencies such as MDT-OCL.)

Comment 3 Daniel Ford 2010-05-04 12:40:45 EDT

Created attachment 166987 [details]
org.openhealthtools.mdht.uml.cda

Missing project

Comment 4 Daniel Ford 2010-05-04 12:43:28 EDT

Casper,
	thanks for looking at the bug.  I mistakenly missed adding the "cda" project to the zip file when I first exported the projects, my apologies.  I have attached another archive with the missing project.
	
	I've been able to reproduce with with native CDO versions of the projects, so it doesn't seem to have anything to do with legacy mode.
	Dan

Comment 5 Daniel Ford 2010-05-04 13:14:48 EDT

A possible clue is that for the attached EMF models, "Initialize by Loading" is "true" in their Genmodels.  It seems like the exception is thrown on the server and then also reported by the client.  Just a guess, but could the problem be that the server is unable to initialize the EPackage "by loading?"

Comment 6 Daniel Ford 2010-05-04 13:54:39 EDT

Another clue, I noticed that the method "createResource(String uri)" is overridden in CDAPackageImpl to use "CDA.xsd" instead of the passed URI.  This would explain a few things...I'll investigate with the model author.

Comment 7 Daniel Ford 2010-05-04 17:26:43 EDT

I made a bit of progress.  I commented out the "createResource(String uri) code in CDAPackageImpl and the code completed normally.  Interestingly, the legacy version of the code works fine outside of CDO, the problem only surfaces when using CDO with either the legacy or CDO native versions of the mode.  Now, I just need to understand why the override was there.

Comment 8 Eike Stepper 2010-05-09 07:00:10 EDT

Any progress here? 

Martin, if it is *not* a legacy issue (as the summary suggests), please move it back to the inbox.

Comment 9 Martin Fluegge 2010-05-09 10:56:37 EDT

Ah. I remember this one. I recently discussed this with Caspar by mail. Thought that I had solved the issue by cleaning all projects. But checking this again proved me wrong. 

I think this bug is around since safeResolve() was introduced in EMFUtil. But I think that the problem is related to the client side. To be exact, to the overwritten createResource method in CDAPackageImpl, as Daniel pointed out earlier. 
I am not a CDO server expert at all, but the simplicity of the URI handed over to the method call could lead to the problem. If the URI looks more conform to the URI standard it works. E.g. like this
	
 	@Override
	public Resource createResource(String uri) {
		return super.createResource("file://c:/temp/CDA.xsd");
	}

Just a guess that CDO simply can not find the resource if the URI is "CDA.xsd". So the package cannot be deployed to the server.

Because Daniel was able to reproduce this with a native and legacy version of the model this could not be a legacy issue. So I assigned it back to the inbox.

Comment 10 Eike Stepper 2010-05-09 12:08:12 EDT

I don't understand where the createResource() method comes from. EMF does not seem to generate it into packages and IMO it wouldn't make sense to have a resource factory in a package as these concerns are orthogonal. Can somebody elaborate please?

Comment 11 Eike Stepper 2010-05-09 12:09:40 EDT

Okay, I found it in EPackageImpl! But who overwrote it and why? Does the generator do that?

Comment 12 Daniel Ford 2010-05-10 13:36:41 EDT

Eike, hi, sorry for the delay, just got back home after a long road trip....and an email gap since last Wednesday...thanks for looking into this one.  Some background...

The models are from the Model Driven Health Tools (MDHT) project of the OpenHealth Tools Foundation https://mdht.projects.openhealthtools.org/  I'm not the author, but I work with them, my main focus is adapting the model to CDO.  I'm discovering the models' attributes as I proceed.

The model is for a "Clinical Document" (i.e., a medical health record).  The line below is the top-level element of the XML serialization of the document.

	<ClinicalDocument xmlns="urn:hl7-org:v3" xmlns:voc="urn:hl7-org:v3/voc" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="urn:hl7-org:v3 CDA.xsd">
	
You'll notice that the URI for the document is "urn:hl7-org:v3" and that the specified "schemaLocation" is "CDA.xsd"  The override of createResource() substitutes "CDA.xsd" for the package's URI of "urn:hl7-org:v3".  I'm not sure that this is absolutely necessary, and unfortunately the author is out sick at the moment, so I can't clarify.

Comment 13 Daniel Ford 2010-05-12 13:43:50 EDT

I've been playing around a bit and am now getting different errors.  I commented out the createResource() override and that allowed things to progress to the point that I got a error about legacy mode not being enabled.  So, I then enabled legacy mode and now I'm getting a NPE from the server.  It seems that a featureId is being read as -1 on the server and that results in NULL being returned when trying to obtain the feature.  

It seems that this model is "problematic," which can be a good thing or a bad thing.  My question right now is how would the CDO developers prefer to see bug reports?  I've seen the server throw four different exceptions when the client tries to commit a single object from this model.  I could create four different reports, but really, they're all related to the fact that this model is "trouble" :-)  Maybe we should change the name of the bug to something more generic for this model and then work through the issues in one place.  For instance, something like "CDO Server throws exceptions for MDHT model."

Dan

java.lang.NullPointerException
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDOList(CDODataInputImpl.java:420)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.readValues(BaseCDORevision.java:653)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.read(BaseCDORevision.java:146)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDORevision(CDODataInputImpl.java:376)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicatingCommit(CommitTransactionIndication.java:226)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicating(CommitTransactionIndication.java:163)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicating(CommitTransactionIndication.java:118)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.indicating(IndicationWithMonitoring.java:84)
	at org.eclipse.net4j.signal.IndicationWithResponse.doExtendedInput(IndicationWithResponse.java:90)
	at org.eclipse.net4j.signal.Signal.doInput(Signal.java:311)
	at org.eclipse.net4j.signal.IndicationWithResponse.execute(IndicationWithResponse.java:63)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.execute(IndicationWithMonitoring.java:63)
	at org.eclipse.net4j.signal.Signal.runSync(Signal.java:238)
	at org.eclipse.net4j.signal.Signal.run(Signal.java:146)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:898)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:920)
	at java.lang.Thread.run(Thread.java:736)

Comment 14 Martin Fluegge 2010-05-12 15:01:24 EDT

Hi Dan,

in a previous comment you mentioned that commenting out the createResource() leads to a correct execution of the code. Has something changed since then?

Could you try running the some code in native mode and check if the NPE still occurs? If so, I would prefer to handle this in a seperate bugzilla. Maybe the other exceptions you dicscovered do not have a common root, so it would be better to handle them seperately. 

But I leave it up to Eike to decide who to proceed from here ;)

Comment 15 Daniel Ford 2010-05-12 15:09:28 EDT

Martin,
	my apologies, I should have been more precise.  Commenting out the createResource() simply lead to a different exception, one complaining about legacy mode not being enabled.  Once I enabled that, I got the NPE.  Basically, one step forward, one step back.  I'll be talking with the model author this afternoon to better understand the need for the createResrouce() override.  
	
	This is with a native CDO version of the model that I regenerated.  The legacy complaint was about the ecore XML model.  That one surprised me, but I'm such a CDO newbie, I just enabled legacy mode and tried to make progress hoping I would understand/learn more in the process.

Comment 16 Martin Fluegge 2010-05-12 15:33:14 EDT

Dan,

if CDO complains that legacy is not enabled (throws LegacyModeNotEnabledExeption) then you tried to use a legacy object with the current view/transaction. 

So there must be something non CDO native in your model if this Exception occurs.

I think I should improve the validity of the stack trace by passing the object to the Exception. This would give us a better hint which object causes the trouble.

Just a node for me. Is the project attached to this bugzilla similar to the one in Bug 310029? Cause I have already managed to install this. ;)

Comment 17 Daniel Ford 2010-05-12 16:07:37 EDT

Martin,
	yes, the project is similar to that in bug 310029, if I remember correctly that one was using the legacy version of the MDHT model and this bug is using a CDO native version I generated.  The "test" code is basically the same snippet I used to try to get things to work (with parts commented out).  I'm trying hard not to confuse the issue to be fixed here, but I thought my explorations might be useful to communicate.
	
	I spoke with John (cc'd on the bug) a few moments ago about the createResource() override and he said that was done on advice from Ed Merks.  I'm looking into exactly why that is needed and what alternatives might be possible.  
	
	If I remember correctly from my investigations from last week, the problem on the server was the that the correct name space URI was being used as the key for the package in the package registry on the server, but that the override was causing a different key to be used to retrieve it during proxy resolution.  This may not be CDO problem, but the code runs with no issues outside of CDO.
	
	Dan

Comment 18 Daniel Ford 2010-05-14 17:33:46 EDT

I think I have it figured out, and it looks like a fundamental issue for CDO, but one that looks fixable.

After working through the problems described above, I was able to progress to the point of deserializing an XML document to create a model instance, and then committing that instance to a CDO repository.  At that point I was getting a NPE on the server in CDOInputImpl.readCDOList() (line 420).  The problem was that a -1 was being read as the featureID on line 418 and so the feature was not found (NULL). 

The reason -1 was being sent as the featureID is because in CDODataOutputImpl.writeCDOList() (line 354), owner.getFeatureID(innerFeature); is returning -1.  The reason it returns -1 is because the "innerFeature" is not an EStructuralFeature of "owner".  What's going on here?  The problem is that "innerFeature" is extracted from "feature", which is an EStructuralFeature of "owner" and also a FeatureMap (in this case called "mixed").  The code assumes that all entries in a FeatureMap are EStructuralFeatures of the EClass that owns the FeatureMap.  This is often true, but not always.

The problem is that FeatureMaps also allow the special features XMLTYpeDocumentRoot.text, DocumentRoot.cDATA and XMLTypeDocumentRoot.comment (see page 196 of the EMF book, 2nd Edition).  They are used to store text, character data and XML comments deserialized from an XML file (and stored in the FeatureMap called mixed that is added to the EClass).  When these special EStructuralFeatures are processed in a FeatureMap, CDO looks in the set of EStructuralFeatures owned by the EClass that owns the FeatureMap "mixed" and doesn't find them, a -1 is returned from the search.  The -1 is sent to the repository anyway and that is what appears on the other side of the communication causing the NPE.

The attached model allows such mixed content and the deserialized XML file has some...exposing the issue.

Possible fix:
Check for the special EStructuralFeatures and if found, send a special featureID value that identifies which of the three ones to use.  One might resort to using negative integers, for instance -2 for XMLTYpeDocumentRoot.text, -3 for DocumentRoot.cDATA and -4 for XMLTypeDocumentRoot.comment, avoiding -1 as that signals that a featureID wasn't found.  Of course, we woudl define some constants for the actual values used.  On the receiving side, the received featureID would be checked against the constants and the appropriate EStructuralFeature used.

I've run out of time to update the attached code (but it's very close to what's there). I'll do that shortly.

Thoughts?

Comment 19 Daniel Ford 2010-05-18 10:21:26 EDT

Created attachment 168934 [details]
archive 1

Comment 20 Daniel Ford 2010-05-18 10:22:36 EDT

Created attachment 168935 [details]
feature map NPE projects #1

Comment 21 Daniel Ford 2010-05-18 10:23:00 EDT

Created attachment 168936 [details]
feature map NPE #2

Comment 22 Daniel Ford 2010-05-18 10:23:20 EDT

I've been attempting to create a small example that reproduces the bug, but I've been unsuccessful.  The main issue seems to be my ignorance of CDO internals (among other things :-)

I've attached the minimum set of projects required to reproduce the NPE on the server.  They are in:
FeatureMapNPE1.zip and FeatureMapNPE2.zip

1) Start the CDO server.
2) Run com.ibm.almaden.mdht.cdo.example1.CauseBug311290 as a Java Application.

The NPE will be thrown on the server immediately.

The problem is as I described in my previous comments.  When the client processes a "mixed" feature map, it looks up the featureID of features extracted from that map in the EClass of the object owning the FeatureMap.  The problem is that the features that represent XML types (like "text") are not features of the EClass and so a -1 is returned as the featureID.  This is transmitted to the server which uses it to retrieve the feature, but gets NULL instead.  The server then dereferences this NULL value and the NPE results.

I tried to create a small XML Schema and resulting model, but I couldn't get the mixed feature maps to be processed in exactly the same manner, I simply don't understand the details enough to get it to work.

I did pair down the XML file that gets deserialized, it is in com.ibm.almaden.mdht.cdo.example1/resources/Bug311290SampleCDODocument.xml to the minimum needed to reproduce the error.  The "troublesome" elements begin on line 26 ("<name>").  This is a "patient name" (type "PN") and the sub-elements ("given", "family" and "suffix") are placed in the PN's "mixed" feature map along with features named "text" of the class "XMLTYpeDocumentRoot".  Put a breakpoint on line 354 of CDODataOutputImpl and you'll see a "-1" being returned as the featureID when the feature "text" is looked up.

Comment 23 Daniel Ford 2010-05-18 16:19:16 EDT

Created attachment 169011 [details]
minimal EMF model to cause NPE

contains a minimal model created from a very simple XML Schema that when is used to deserliaze an XML instance conforming to the schema causes the CDO Server to throw a NPE

Comment 24 Daniel Ford 2010-05-18 16:19:30 EDT

I managed to create a minimal model that causes the bug to happen.  The model is created from a XML schema consisting of a single element of type "xsd:string".  The generated EMF/CDO code is then used to deserialize a simple XML file with a single comment and a single (empty) element.  Saving the deserialized model instance in a CDO repository causes the server to throw a NPE immediately.  

I've updated the attachments, the model is in the file bug311290.zip.
1) Start the CDO server
2) Run org.eclipse.bug311290.CauseBug311290 as a Java application.

The NPE is thrown immediately.

The schema is in org.eclipse.bug311290/model/bug311290.xsd

The xml file is in org.eclipse.bug311290/resourcesl/bug311290.xml

Comment 25 Eike Stepper 2010-05-19 03:17:03 EDT

Hi Dan,

just a note so that you don't get too disappointed: We appreciate your engagement very much!!! Unfortunately these feature map details are a real hazzle and according to the Helios release schedule I must currently focus on issues that have releng and API impact. I hope that I can support you sometime soon.

Maybe others have more time now?!

Comment 26 Daniel Ford 2010-05-19 13:06:25 EDT

Eike,
	no problem.  I completely understand, maybe you could "bump" its priority so that it comes up for early consideration.
	
I hope the "noise" I generated trying to come up with a succinct example of how to reproduce the bug wasn't too distracting.  It took me a while to get there, but I learned a lot along the way.

IMHO, the bug is pretty significant, basically any model instance deserialized from an XML file can cause the NPE.  In the simplest cases, all that is needed is a comment in the "wrong" place.  It popped up in my first attempts at using CDO because of simple white space in the file.

I think the bug hasn't been seen before because Legacy Mode wasn't available to support the EMF XMLType model.

Comment 27 Eike Stepper 2010-05-19 13:43:51 EDT

(In reply to comment #26)
> I hope the "noise" I generated trying to come up with a succinct example of how
> to reproduce the bug wasn't too distracting.  

No, I developed adequate filters over the years :P

> It took me a while to get there,
> but I learned a lot along the way.

I appreciate that. I hope you do so, too ;-)

> IMHO, the bug is pretty significant, basically any model instance deserialized
> from an XML file can cause the NPE.  In the simplest cases, all that is needed
> is a comment in the "wrong" place.  It popped up in my first attempts at using
> CDO because of simple white space in the file.
> 
> I think the bug hasn't been seen before because Legacy Mode wasn't available to
> support the EMF XMLType model.

I rather think it's because in the last 6 years nobody has used CDO in combination with XML files that contain other things than the model instance itself. XML in combination with CDO is probably mostly used to import/export repositoy data without ever looking into the file, or even modify them. Originally CDO was een not meant to support arbitrary XML constructs. That's why I was already tempted to mark this as an enhancement request ;-)

Comment 28 Daniel Ford 2010-05-20 15:58:07 EDT

Eike, let me explain the use case, and maybe that will help sway you.  The XML files are instances of "Clinical Documents" (i.e., electronic health records).  The MDHT project https://mdht.projects.openhealthtools.org/ has developed EMF models for these documents.  I've been trying to use CDO as the scalability/accessibility solution for access to the deserialized document instances represented as EObjects.  

We want to use CDO to interlink various producers and consumers of these documents.  One scenario we envision is very similar to the CDO restaurant demo with RCP clients for nurses and doctors and other interfaces for medical monitoring devices.  The devices would read patient data and update the health care record, these updates would then propagate to the clients.

Comment 29 Eike Stepper 2010-05-21 03:01:12 EDT

Yes, that sounds indeed very interesting and I didn't intend to think something else ;-)

Comment 30 Daniel Ford 2010-05-27 22:18:34 EDT

With some insight from my colleague John Timm, I managed to make some progress on the bug and implemented a fix.  The key was recognizing that the innerFeature could be from several different EClasses in the XMLTypePackage not just XMLTypeDocumentRoot.  The fix is to extract the classifier id of the innerFeature's EClass as well as the innerFeature's featureID.  These are then encoded together into one "featureID" by multiplying the classifier ID by 100 and then adding in the innerFeature's featureID.  So, if the classifierID is 3 and the featureID is 4, then the result is 304.  This is then negated to differentiate it from any other "normal" featureID, so -304 is sent to the server.  Because it is negative, the server code recognizes that this value is special and encodes both an XMLTypePackage EClass and one it its EStructuralFeatures.  It then extracts the two values and creates the appropriate innerFeature on the server.  

This seems to fix things and the test code that reproduces the bug passes without a problem.

However, for a different case, with a larger XML sourced model instance, I've run into a different problem.  

It seems that the XMLTypePackage is being correctly registered with the CDOPackageRegistry on the client, but that its package definition is not making its way to the server.  This is with legacy mode enabled.  All of the other registered packages all make it to the sever.  The one difference is that the XMLType model is a legacy model (part of EMF) while all of the other packages are native CDO.

Is there something subtle I'm missing that would prevent the EMF XMLType model from getting to the server along with the other models?

Comment 31 Eike Stepper 2010-05-28 01:42:18 EDT

(In reply to comment #30)
> With some insight from my colleague John Timm, I managed to make some progress
> on the bug and implemented a fix.  

Great!

> The key was recognizing that the innerFeature could be from several 
> different EClasses in the XMLTypePackage not just XMLTypeDocumentRoot.  

As I mentioned, I'm not a feature map expert. Is XMLTypePackage the only source for innerFeature types or are custom EPackages also generally possible?

> The fix is to extract the classifier id of the
> innerFeature's EClass as well as the innerFeature's featureID.  These are then
> encoded together into one "featureID" by multiplying the classifier ID by 100
> and then adding in the innerFeature's featureID.  So, if the classifierID is 3
> and the featureID is 4, then the result is 304.  This is then negated to
> differentiate it from any other "normal" featureID, so -304 is sent to the
> server.  Because it is negative, the server code recognizes that this value is
> special and encodes both an XMLTypePackage EClass and one it its
> EStructuralFeatures.  It then extracts the two values and creates the
> appropriate innerFeature on the server.  

Sounds smart. Some thoughts:

a) In general classifierIDs are not guaranteed to be stable when an EPackage is deserialized. We've noticed that for example with UML2 and that was the reason why we turned it into clasifier names in CDOClassifierRef.

b) If other EPackages than XMLTypePackage are possible (see above) we'd need to use CDOClassifierRef which encodes both package and class. And for any custom EPackage/EClass we could probably not expect no more than 100 features per class.

c) Would additional DB columns make sense for EPackage/EClass or is this so special to XMLTypePackage that a special feature encoding is good enough?

> This seems to fix things and the test code that reproduces the bug passes
> without a problem.

Can we use your patch?

> However, for a different case, with a larger XML sourced model instance, I've
> run into a different problem.  
> 
> It seems that the XMLTypePackage is being correctly registered with the
> CDOPackageRegistry on the client, but that its package definition is not making
> its way to the server.  This is with legacy mode enabled.  All of the other
> registered packages all make it to the sever.  

What does "make it to the sever" mean exactly? 

> The one difference is that the
> XMLType model is a legacy model (part of EMF) while all of the other packages
> are native CDO.

Is it possible that you create a small test case so that we can easily reproduce it? There are many example in the cdo.tests plugin.

> Is there something subtle I'm missing that would prevent the EMF XMLType model
> from getting to the server along with the other models?

Not sure, yet. It *could* be a problem with the new legacy mode. A test case will help to investigate it. Thanks for all your effort ;-)

Comment 32 Daniel Ford 2010-05-28 11:52:14 EDT

Eike,
I should clairify that I consider the "fix" to be a temporary "hack."  It works for me, for now, but something more general may really be necessary, because, as you point out, there could be EStructuralFeatures from other packages than just XMLType.  I also would love to provide a patch, but the policies of my current employer require a long process of approvals for "patches", which are considered to be "code contributions", a small example that reproduces a bug, on the other hand, is not,...for the moment, its faster/easier to just describe it.  If I come up with a real fix, I'll push it through the system.   Sigh.

c) I'm not sure about extra DB columns, I simply don't know enough about CDO internals yet to say.  

The problem I hit after getting over the featureMap issue is that the package for XMLType is not in the CDORegistry on the server, this causes a failure that complains about not being able to find a "meta id."  I've stepped through things on the server and find all of the other packages, that I register on the client, in the registry on the server, but not XMLType (that's what I mean by "not making it to the server").  This causes the lookup failure.

I'll spend some time today looking at how the other packages are migrating and see what's stopping XMLType from making the trip.

BTW, I can sympathize with you being  "not a feature map expert."  They didn't make much sense to me either until I plunged into how EMF processes XML.  They exist to record the sequence of things parsed from an XML file.  They basically consist of a list of entries that map an EStructuralFeature instance to a value.  The idea is that the order of the EStructuralFeatures in the feature map list matches the sequence in which their values were deserialized from an XML file.  If the model instance is later serialized by writing the values in the order that they appear in the feature map, then they'll match the order in which they were deserialized.   The problem that causes this particular bug, is that EMF want's to preserve things like XML comments and text (and other things) as well as the values of deserialized elements, and it wants to preserve the order in which they appear as well.  To do that, it injects EStructuralFeature instances from the EClass XMLTypeDocumentRoot (among others) into the feature map to record the order and sequence of their appearance in the XML file.  

A potential for confusion about feature maps is that there are other "features" in a class that represent the values that are in the feature map.  The trick is to understand that, is that those features do not store their values, only the feature map does.  Instead, there is a generator pattern that causes access of those features to look into the feature map and use a utility to filter out only those entries that match their particular EStructureFeature, and ignore the rest.  These are then returned as the value of the "feature".

Comment 33 Daniel Ford 2010-05-28 16:34:34 EDT

I made some more progress.  The XMLType package is not sent to the server because none of the "new objects" have it as a package.  I'm not quite sure what is going on exactly, but it seems like the method CDOStateMachine.getProperContents() is excluding any instances of the XMLType package.

Comment 34 Eike Stepper 2010-05-29 06:29:56 EDT

Yes, I can well imagine that containment through feature maps is not properly dealt with ;-(

Comment 35 Daniel Ford 2010-06-01 12:22:50 EDT

I tried a work around where I had the server load the XMLType package just like it initially loads the Ecore and Eresource  packages.  This fixed the complication of discovering that they should be sent from the client.  That seemed to work because I got a different exception :-)

Maybe a interpretation of this exception will help me make some progress.  The Patient class is the troublesome one because it allows xml text, something that caused trouble because it made the featureMaps a bit more complicated.  What would cause its "Revision" not to be registered?  


org.eclipse.emf.cdo.util.CommitException: java.lang.IllegalStateException: Revision was not registered: Patient@OID5:0v1
	at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.commit(CDOTransactionImpl.java:918)
	at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.commit(CDOTransactionImpl.java:929)
	at com.ibm.almaden.mdht.cdo.example1.WriteCDAs.main(WriteCDAs.java:108)

Comment 36 Daniel Ford 2010-06-10 13:45:06 EDT

I worked a bit more on this and understand things a bit better.  The exception is thrown in CDOStateMachine.execute() when in an attempt to  add a revision to the Session's RevisionManager returns false, indicating that the revision was not added.  Stepping into the addRevision() method, in CDORevisionManagerImpl, it returns false if an attempt to add to the cache ("cache.addRevision(revision);") returns false.  It turns out that that method simply returns the result of (the internal class) RevisionList.addRevision(...).  Stepping into that method, I discovered that it returns false if it finds that version of the revision is already in the list, otherwise it either updates a previous version, or adds the new one.  

So, the semantics of the return values seem to be inconsistently interpreted has they head back up the stack.  At the bottom, false means that the thing was already registered, so it wasn't "added."  At the top, false indicates that the revision is not being added, which is true, it is not being added AGAIN, because it is already registered, but instead this triggers an exception saying that the revision isn't registered (I guess because the "add" failed), which is not actually the case.

So, I commented out the code in CDOStateMachine that throws the exception and suddenly "everything" works.

Comment 37 Eike Stepper 2010-06-29 04:54:36 EDT

Rebasing all outstanding 3.0 problem reports to version 3.0.1

Comment 38 Eike Stepper 2010-06-29 12:25:21 EDT

Dan, Sorry for the delay. Is it possible that you attach a (workspace-rooted) patch for your simple change? That would make it easier for me to find what you mean...

Comment 39 Daniel Ford 2010-06-29 14:19:04 EDT

Eike,
	thanks for looking at this.  Your timing is very good, I had put it aside for a while and was just looking at it again today.  I had to make at least three changes to the code to get something to work, and then it was very fragile.  I think it might be best to look at the changes I made in sequence as they address three different issues.
	
	I'll attach a patch for the first change I made to address the main problem which is that handling of features maps needs to be more generic.  A FeatureMap is exactly like it sounds, a map between EStructuralFeature instances and values.  The current implementation assumes that all of the EStructuralFeature instances will belong to the EClass that contains the FeatureMap.  This is not true in general, in fact, the EStructureFeatures can come from EClass instances that are in completely different packages (e.g., XMLType).  The attached patch is a hack that uses the integer that identifies the EStructuralFeature to encode both the EClass and EStructureFeature.  However, its flaw is that it assumes that if the EStructuralFeature is not in the EClass owning the FeatureMap, then it must be in a class in the EMF package XMLType.  This is often true so the patch/hack works for some cases, but it is not true in general.
	I think what is needed is a way to encode an arbitrary EPackage reference (nsURI?), an EClass within it, and an EStructuralFeature within that EClass (i.e., more than can be encoded in a single integer).  It was at this point that I decided it was time for expert help :-)
	A related (second) issue is that once the encoding is working, one runs into the fact that the EPackage XMLType is not available to the server.  I found the place where Ecore package is loaded and added some code to also load XMLType.
	
	Dan

Comment 40 Daniel Ford 2010-06-29 14:20:41 EDT

Created attachment 173032 [details]
Encoding of EStructureFeature and EClass

assumes EStructuralFeatures come from the current package or XMLType.

Comment 41 Eike Stepper 2010-07-07 07:10:28 EDT

Fixing wrong bug version.

Comment 42 Eike Stepper 2010-07-15 02:56:12 EDT

(In reply to comment #39)
> Eike,
>     thanks for looking at this.  Your timing is very good, I had put it aside
> for a while and was just looking at it again today.  I had to make at least
> three changes to the code to get something to work, and then it was very
> fragile.  I think it might be best to look at the changes I made in sequence as
> they address three different issues.
> 
>     I'll attach a patch for the first change I made to address the main problem
> which is that handling of features maps needs to be more generic.  A FeatureMap
> is exactly like it sounds, a map between EStructuralFeature instances and
> values.  The current implementation assumes that all of the EStructuralFeature
> instances will belong to the EClass that contains the FeatureMap.  This is not
> true in general, in fact, the EStructureFeatures can come from EClass instances
> that are in completely different packages (e.g., XMLType).  The attached patch
> is a hack that uses the integer that identifies the EStructuralFeature to
> encode both the EClass and EStructureFeature.  However, its flaw is that it
> assumes that if the EStructuralFeature is not in the EClass owning the
> FeatureMap, then it must be in a class in the EMF package XMLType.  This is
> often true so the patch/hack works for some cases, but it is not true in
> general.
>     I think what is needed is a way to encode an arbitrary EPackage reference
> (nsURI?), an EClass within it, and an EStructuralFeature within that EClass
> (i.e., more than can be encoded in a single integer).  It was at this point
> that I decided it was time for expert help :-)

We are not restricted to a simple integer in the protocol. You can add additional transfers like writeCDOClassifierRef()/readCDOClassifierRefAndResolve(). Do you want to try that out?

>     A related (second) issue is that once the encoding is working, one runs
> into the fact that the EPackage XMLType is not available to the server.  I
> found the place where Ecore package is loaded and added some code to also load
> XMLType.

I think the cause for this is that hte client side mechanism for detecting needed EPackages is not ideal for such feature maps. Maybe you want to have a look at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.analyzeNewPackages()

Comment 43 Daniel Ford 2010-07-19 13:08:32 EDT

Eike,
	I made a small change to follow your advice producing the code snippet below for the "write" and a simliar one for the "read."  However, I'm now getting the exception on the server pasted below.  I'm unable to interpret it.  Could you give me some insight please?
	
	Thanks
	
	Dan
	
	 if (isFeatureMap)
      {
        Entry entry = (FeatureMap.Entry)value;
        innerFeature = entry.getEStructuralFeature();
        value = entry.getValue();

        final EClass featuresClass = innerFeature.getEContainingClass();
        writeCDOClassifierRef(featuresClass);

        int featureID = featuresClass.getFeatureID(innerFeature);

        writeInt(featureID);
      }
	
	
[ERROR] Not active: SignalProtocol[cdo]
java.lang.IllegalStateException: Not active: SignalProtocol[cdo]
	at org.eclipse.net4j.util.lifecycle.LifecycleUtil.checkActive(LifecycleUtil.java:71)
	at org.eclipse.net4j.signal.Signal.getProtocol(Signal.java:88)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.getProtocol(CommitTransactionIndication.java:87)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication$1.getPackageURICompressor(CommitTransactionIndication.java:131)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDOPackageURI(CDODataInputImpl.java:141)
	at org.eclipse.emf.cdo.internal.common.model.CDOPackageInfoImpl.read(CDOPackageInfoImpl.java:104)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDOPackageInfo(CDODataInputImpl.java:135)
	at org.eclipse.emf.cdo.internal.common.model.CDOPackageUnitImpl.read(CDOPackageUnitImpl.java:302)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDOPackageUnit(CDODataInputImpl.java:106)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicatingCommit(CommitTransactionIndication.java:211)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicating(CommitTransactionIndication.java:165)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicating(CommitTransactionIndication.java:120)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.indicating(IndicationWithMonitoring.java:84)
	at org.eclipse.net4j.signal.IndicationWithResponse.doExtendedInput(IndicationWithResponse.java:90)
	at org.eclipse.net4j.signal.Signal.doInput(Signal.java:315)
	at org.eclipse.net4j.signal.IndicationWithResponse.execute(IndicationWithResponse.java:63)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.execute(IndicationWithMonitoring.java:63)
	at org.eclipse.net4j.signal.Signal.runSync(Signal.java:240)
	at org.eclipse.net4j.signal.Signal.run(Signal.java:146)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:898)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:920)
	at java.lang.Thread.run(Thread.java:736)

Comment 44 Eike Stepper 2010-07-19 13:15:17 EDT

That's strange. It's either the result of a test case ending and closing the session while a background thread is still using it or it may also be an indication that you somehow messed up the stream between client and server.

The latter means that you're not reading data out of a stream in the exact same order as it has been written to it. This can even result in wrong signals being executed in the receiver following the signal that caused the damage.

Does this help?

Comment 45 Daniel Ford 2010-07-19 14:39:03 EDT

I take all the blame :-)  Here're the two sides of the exchange.  Do you see anything obvious?

Write
-------

    CDOIDProvider idProvider = getIDProvider();
    boolean isFeatureMap = FeatureMapUtil.isFeatureMap(feature);
    for (int j = 0; j < size; j++)
    {
      Object value = list.get(j, false);
      EStructuralFeature innerFeature = feature; // Prepare for possible feature map
      if (isFeatureMap)
      {
        Entry entry = (FeatureMap.Entry)value;
        innerFeature = entry.getEStructuralFeature();
        value = entry.getValue();

        final EClass featuresClass = innerFeature.getEContainingClass();
        writeCDOClassifierRef(featuresClass);

        int featureID = featuresClass.getFeatureID(innerFeature);

        writeInt(featureID);
      }

      if (value != null && innerFeature instanceof EReference)
      {
        value = idProvider.provideCDOID(value);
      }

      if (TRACER.isEnabled())
      {
        TRACER.trace("    " + value); //$NON-NLS-1$
      }

      writeCDOFeatureValue(innerFeature, value);
    } // for
    
    
 Read
 ------
 
    InternalCDOList list = (InternalCDOList)getListFactory().createList(size, size, referenceChunk);
    for (int j = 0; j < referenceChunk; j++)
    {
      if (isFeatureMap)
      {
        EClass featuresClass = (EClass)readCDOClassifierRefAndResolve();

        int featureID = readInt();
        EStructuralFeature innerFeature = featuresClass.getEStructuralFeature(featureID);

        type = CDOModelUtil.getType(innerFeature.getEType());
        value = type.readValue(this);
        value = CDORevisionUtil.createFeatureMapEntry(innerFeature, value);
      }
      else
      {
        value = type.readValue(this);
      }

      list.set(j, value);
      if (TRACER.isEnabled())
      {
        TRACER.trace("    " + value); //$NON-NLS-1$
      }
    }

Comment 46 Eike Stepper 2010-07-20 01:42:49 EDT

Can you attach a patch of your changes? That would make it easier for me ;-)

Comment 47 Daniel Ford 2010-07-27 09:02:04 EDT

Sorry for the delay.  I've attached a patch.  The main changes are in CDODataInputImpl.java and CDODataOutputImpl.java where the contents of the feature map are transmitted.

The other changes are peripheral, in that they fix other issues not directly related to the proper transmission of the fetaure map.  The first is a change to CDOStateMachine.java where I commented out the throwing of an exception, this fixes a bug I mentioned previously.  The other change is in Repository.java where I initialize the XMLTypePackage.

Comment 48 Daniel Ford 2010-07-27 09:03:07 EDT

Created attachment 175309 [details]
changes to transmit FeatureMap contents.

Comment 49 Eike Stepper 2010-07-27 10:25:12 EDT

(In reply to comment #47)
> The other change is in
> Repository.java where I initialize the XMLTypePackage.

I didn't have the time yet to look at your new patch but you don't mean that you a particular non-system model (XMLTypePackage) in Repository.java, do you? Does that mean we have to adjust the framework each time someone needs a new special package? What if that package is not part of EMF?

Or are you just doing that to demonstrate something?

Comment 50 Daniel Ford 2010-07-28 11:09:05 EDT

I had to add XMLTypePackage because it wasn't being sent to the server.  I think this is mostly a hack because in general, I think CDO may need to be able to handle any EPackage instance.  The basic problem I think is that in general a FetaureMap can have an instance of any EStructurealFeature from any EPackage as a key.  I managed to get things to "work" when the features were restricted to XMLTypePackage, but when I expanded my XML deserialization, this introduced features from other (non-EMF) packages which I couldn't encode in the single integer being sent to the server.

Comment 51 Daniel Ford 2011-01-18 13:13:12 EST

Are there any plans to give this bug some attention?

Thanks 
Dan

Comment 52 Eike Stepper 2011-01-26 14:44:07 EST

Hi Daniel, I'm willing to give it a new try, but unfortunately much has happened in the meantime and I can't apply your patches to the new SVN trunk anymore. Is it possible that you adjust the needed patches and combine them into a single workspace-relative patch? Please mark the unneeded attachments as obsolete.

Comment 53 Daniel Ford 2011-02-14 10:54:45 EST

Eike,
	the patches are not really that important or useful, they were my attempts at hacking a temporary workaround.  The real problem is that CDO doesn't support EMF feature maps.  This manifests itself when working with deserialized model instances that conform to an XML schema.  You said previously (comment 27, May 19, 2010) that this was outside the scope of CDO.  I think most users will find this surprising as such scenarios are a major use case for standard EMF.  Anyway, I understand the issues of limited resources and time force the prioritization of everything, I just want to add my vote to increasing the visibility of this bug.  

Dan

Comment 54 Eike Stepper 2011-06-23 04:27:39 EDT

Moving all open problem reports to 4.0

Comment 55 Marco Descher 2012-05-20 09:49:35 EDT

Hy,

I am currently working at the exactly same thing (mdht openhealth datatypes), facing the same problem! Daniel, have your finally brought this usably running? At certain parts I am having a hard time to completely understand the problem, but I am simply tempted to find the conflicting parts and do an alternate modelling. Dou you possibly see what modifications would have to be done to the ecore to be usable with the current CDO version?

Thanks a lot!
Marco

PS: For the sake of completeness, with the newest integration build the stack trace for the problem is as follows:

java.lang.NullPointerException
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDOList(CDODataInputImpl.java:546)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.readValues(BaseCDORevision.java:910)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.read(BaseCDORevision.java:154)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDORevision(CDODataInputImpl.java:487)
	at org.eclipse.emf.cdo.internal.common.protocol.CDODataInputImpl.readCDORevision(CDODataInputImpl.java:478)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicatingCommit(CommitTransactionIndication.java:171)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CommitTransactionIndication.indicating(CommitTransactionIndication.java:95)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.CDOServerIndicationWithMonitoring.indicating(CDOServerIndicationWithMonitoring.java:109)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.indicating(IndicationWithMonitoring.java:84)
	at org.eclipse.net4j.signal.IndicationWithResponse.doExtendedInput(IndicationWithResponse.java:90)
	at org.eclipse.net4j.signal.Signal.doInput(Signal.java:326)
	at org.eclipse.net4j.signal.IndicationWithResponse.execute(IndicationWithResponse.java:63)
	at org.eclipse.net4j.signal.IndicationWithMonitoring.execute(IndicationWithMonitoring.java:63)
	at org.eclipse.net4j.signal.Signal.runSync(Signal.java:251)
	at org.eclipse.net4j.signal.Signal.run(Signal.java:147)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	at java.lang.Thread.run(Thread.java:680)

Comment 56 Eike Stepper 2012-06-05 07:30:38 EDT

Moving all open bug reports to 4.1 because the release is very near and it's hghly unlikely that there will be spare time to address 4.0 problems.

Please make sure that your patches can be applied against the master branch and that your problem is not already fixed there!!!

Comment 57 Eike Stepper 2012-08-14 22:53:48 EDT

Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master.

Comment 58 Eike Stepper 2013-06-29 12:19:16 EDT

We'll try to address open problems in 4.3 (master) first and then port fixes back to 4.2.

Comment 59 Eike Stepper 2015-07-14 02:15:37 EDT

Moving all open bugzillas to 4.5.

Comment 60 Eike Stepper 2016-07-31 00:58:42 EDT

Moving all unaddressed bugzillas to 4.6.

Comment 61 Eike Stepper 2016-09-24 01:41:09 EDT

We have no plans to support feature maps.