Community
Participate
Working Groups
Build Identifier: 3.6.2 When a new element is added to the XML model, if the file that is being edited is a DTD, the XmlContentModel is not properly loaded so the proper insertion point is not found and new elements are always appended to the end of a childNode list regardless of what is required in the DTD. Reproducible: Always Steps to Reproduce: 1. Create model for xml file that has DTD 2. Create two elements in XML file that have specific ordering 3. Delete all instances of first element 4. Add a new element that should come first in childNode list 5. Notice that the element is appended at the end and causes an error in DTD validation.
Created attachment 192901 [details] First patch attempt This is a small patch that does 2 things that seems to fix the problem at least in my workspace. 1. In XmlDocumentSchemaCache.java, flips the order of the arguments being passed to DtdParser.parseFromUrl to match the intended use as per the API. 2. In XmlElementDefinition.java if the content model can't be found from importedSchemalocation since its null (i guess because we are in a DTD file and there is no imported schemas), then simply return the schema that was passed in during created of XmlElementDefinition I don't have a way to test editing an XML file backed by an XSD so I don't know if my fix would break that scenario.
Could you indicate the version of Sapphire that you are using and what branch the patch is for? I will take a look on Monday.
The version of Sapphire that I was using is: v0.3.0.201104071718 I installed this from this update-site just a few days ago: https://hudson.eclipse.org/hudson/job/sapphire-0.3.x/lastStableBuild/artifact/build/repository/ And the patch that I created I made by checking out the org.eclipse.sapphire.modeling.xml plugin from CVS HEAD and then creating a patch against that.
Starting to look at this and want to clarify something... How do you define the root element binding.... Did you implement a custom root element controller (@CustomXmlRootBinding)? I just noticed that @XmlRootBinding annotation doesn't account for DTDs...
Created attachment 192967 [details] Patch v2 > 1. In XmlDocumentSchemaCache.java, flips the order of the arguments being > passed to DtdParser.parseFromUrl to match the intended use as per the API. This is the correct description, although the original patch modified invocation of XSD parser rather than the DTD parser. > 2. In XmlElementDefinition.java if the content model can't be found from > importedSchemalocation since its null (i guess because we are in a DTD file and > there is no imported schemas), then simply return the schema that was passed in > during created of XmlElementDefinition This is pretty close to being a correct fix. The issue is that namespace is "" and this.schema.getNamespace() is null. The check for "is this a reference to the current element's schema" on line 63 failed to account for this. I fixed that. These are regressions from 0.2 are due to a recent re-write of DTD parsing. It looks like we didn't have unit test coverage in this area. I added TestXmlDtd0003 to cover this scenario. I also found that you cannot use @XmlRootBinding with a DTD... Well, if you use @XmlRootBinding( elementName = "abc" ), it would work if you open an existing document with a DTD declaration, but it wouldn't create the DTD declaration if starting from scratch. So far at Oracle, we've only used @CustomXmlRootBinding in cases where DTDs were involved. I added a small enhancement that allows declaration like this one: @XmlRootBinding( elementName = "root", schemaLocation = "TestXmlDtd0003.dtd" ) which ends up producing the following in the XML document: <!DOCTYPE root SYSTEM "TestXmlDtd0003.dtd"> This isn't really a long-term fix. I will open an enhancement request to cleanup root xml binding API to cleanly account for different XML content models.
Released Patch v2. Thanks for diagnosing the problem and providing the first cut at the fix. I am very impressed that you were able to dive into the framework internals like this to find the problem. I look forward to hearing more about how you use Sapphire and maybe seeing more contributions in the future.
Bug 342487 - Improve @XmlRootBinding API
(In reply to comment #4) > Starting to look at this and want to clarify something... How do you define the > root element binding.... Did you implement a custom root element controller > (@CustomXmlRootBinding)? I just noticed that @XmlRootBinding annotation doesn't > account for DTDs... I am using @XmlRootBinding but I was opening an existing XML file that has the DTD declaration and some sample/default content. I hadn't tried going from a brand new or blank file. Going forward, with my DTD based XML do I need to use @CustomXmlRootBinding or can I safely stick with @XmlRootBinding
(In reply to comment #5) > Created attachment 192967 [details] > Patch v2 > > > 1. In XmlDocumentSchemaCache.java, flips the order of the arguments being > > passed to DtdParser.parseFromUrl to match the intended use as per the API. > > This is the correct description, although the original patch modified > invocation of XSD parser rather than the DTD parser. > > > 2. In XmlElementDefinition.java if the content model can't be found from > > importedSchemalocation since its null (i guess because we are in a DTD file and > > there is no imported schemas), then simply return the schema that was passed in > > during created of XmlElementDefinition > > This is pretty close to being a correct fix. The issue is that namespace is "" > and this.schema.getNamespace() is null. The check for "is this a reference to > the current element's schema" on line 63 failed to account for this. I fixed > that. > > These are regressions from 0.2 are due to a recent re-write of DTD parsing. It > looks like we didn't have unit test coverage in this area. I added > TestXmlDtd0003 to cover this scenario. > > I also found that you cannot use @XmlRootBinding with a DTD... Well, if you use > @XmlRootBinding( elementName = "abc" ), it would work if you open an existing > document with a DTD declaration, but it wouldn't create the DTD declaration if > starting from scratch. So far at Oracle, we've only used @CustomXmlRootBinding > in cases where DTDs were involved. > > I added a small enhancement that allows declaration like this one: > > @XmlRootBinding( elementName = "root", schemaLocation = "TestXmlDtd0003.dtd" ) > > which ends up producing the following in the XML document: > > <!DOCTYPE root SYSTEM "TestXmlDtd0003.dtd"> > > This isn't really a long-term fix. I will open an enhancement request to > cleanup root xml binding API to cleanly account for different XML content > models. Very interesting, thank you for all of these details. Here is my current declaration: @XmlRootBinding(elementName = "service-builder") Can I use this? @XmlRootBinding(elementName = "service-builder", schemaLocation = "http://www.liferay.com/dtd/liferay-service-builder_6_0_0.dtd") For reference here is the correct full doctype of my XML file. <!DOCTYPE service-builder PUBLIC "-//Liferay//DTD Service Builder 6.0.0//EN" "http://www.liferay.com/dtd/liferay-service-builder_6_0_0.dtd">
> Can I use this? > > @XmlRootBinding(elementName = "service-builder", schemaLocation = > "http://www.liferay.com/dtd/liferay-service-builder_6_0_0.dtd") > > For reference here is the correct full doctype of my XML file. > > <!DOCTYPE service-builder PUBLIC "-//Liferay//DTD Service Builder 6.0.0//EN" > "http://www.liferay.com/dtd/liferay-service-builder_6_0_0.dtd"> At the moment, this declaration should produce the following DOCTYPE: <!DOCTYPE root SYSTEM "http://www.liferay.com/dtd/liferay-service-builder_6_0_0.dtd"> Better support for various details of DTD/DOCTYPE declarations will need to wait for Bug 342487 as we will need new API to specify the various PUBLIC, SYSTEM, etc options. You can experiment with @CustomXmlRootBinding instead. That API gives you full control over how the root element and the corresponding header information is created.
(In reply to comment #6) > Released Patch v2. Thanks for diagnosing the problem and providing the first > cut at the fix. Will I be able to get this update via this updatesite or is there a better/more appropriate one to use? https://hudson.eclipse.org/hudson/job/sapphire-0.3.x/lastStableBuild/artifact/build/repository/ > > I look forward to hearing more about how you use Sapphire and maybe seeing more > contributions in the future. I have plans for using Sapphire to implement editors for several custom XML files that my company uses. I hope to be able to give back to the project by filing bug reports and contributing patches.
> Will I be able to get this update via this updatesite or is there a > better/more appropriate one to use? > > > https://hudson.eclipse.org/hudson/job/sapphire-0.3.x/lastStableBuild/artifact/build/repository/ The fix is in build 253 or newer. The above URL will see it. Note that I don't recommend updating from one integration build to another. Uninstall the old build first.
Oh and forgot to mention... Once you get the new build and confirm the fix, please make a note of that here so that we know the issue is verified and can close this bug.
I verified in a new build that this is now fixed. But I also noticed that my Condition class for if an Action should be added to a context in sapphire-extension.xml is now not being called when in the previous build it was being called. But I'll followup with that issue later.
> I verified in a new build that this is now fixed. Thanks. Closing as fixed. > But I also noticed that my Condition class for if an Action should be added to > a context in sapphire-extension.xml is now not being called when in the > previous build it was being called. But I'll followup with that issue later. I too noticed this as the result of an earlier unrelated code change. Working on a fix now.
The issue with extensions not loading should be resolved in build 258. If you are still having problems with this, please open a bug.
(In reply to comment #16) > The issue with extensions not loading should be resolved in build 258. If you > are still having problems with this, please open a bug. I just tried the latest build, my action extensions including the Condition check are working fine now. Thanks!
