Community
Participate
Working Groups
To make Sapphire Xml Binding to support more details of DTD which includes the publicId, systemId and rootElementName. The approach right now uses the @XmlRootBinding and checks for the DTD inside the StandardRootElementController, which is ambigous and unknown to the user. The same issue has been discussed in the fourm post http://www.eclipse.org/forums/index.php/t/235837/
Created attachment 201966 [details] Analysis and New Classes
What is in the attachment? Could you attach individual files or at least uses .zip format? I have no means to open a .rar file. Note that if you intend to propose a patch, you will need to attach it in patch format via bugzilla facilities. An archive of files cannot be accepted per Eclipse Foundation rules.
I got that i need to use patch mechanism for submitting or contributing the solution, i just thought could share my files intially as part of anyalysis to that i dont do the Y <<<< X ! Once you are fine with the implementation i shall inlcude them in the Sapphrie source tree and write test case and document and send you a patch. Hope I have sense
Created attachment 202046 [details] For Preview These files attached is for your preview and if you feel fine i shall send you the patch
I think there are two separate concerns: (a) specifying the root element binding and (b) specifying xml schema or dtd. I think what would make sense here is to use @XmlRootBinding annotation together with another annotation that specifies the DTD. @XmlRootBinding( elementName = "liferay-portlet-app" ) @XmlDtd( publicId = "-//Liferay//DTD Portlet Application 6.0.0//EN", systemId = "http://www.liferay.com/dtd/liferay-portlet-app_6_0_0.dtd" ) To make this match on XML Schema side, we would retire @XmlRootBinding.schemaLocation and replace with @XmlSchema( namespace, location ). The same StandardRootElementController should handle both cases.
Maybe @XmlDocType for the new annotation
exactly, @XmlDocType is a nice name, What I am thinking of is introducing a RootElementControllerFactory which will build the StandardRootElementController or DtdRootElementController based on the @XmlSchema or @XmlDoctype I alo feel that @XmlRootBinding should retain the other elements for backward compatibility sake, where some of the adopters are using it and might have annotated the classes with them so we need to give some time to them and slowly we shall do it, but for now we shall introduce the classes alone and will say to adpoters in the doc that we will be retiring the older form in future versions. Will try to make patch with all these changes and send it for your review! Hope your review is shorter than previous time for trivial issues and we could further brainstrom Hope I make sense!
> What I am thinking of is introducing a RootElementControllerFactory which will > build the StandardRootElementController or DtdRootElementController based on > the @XmlSchema or @XmlDoctype We probably don't need a factory. A simple if statement in RootXmlResource should do the trick. > I alo feel that @XmlRootBinding should retain the other elements for backward > compatibility sake Backwards compatibility is important for established projects, but #1 priority for Sapphire right now is evolving the API. This means no backwards compatibility between major releases (0.3 to 0.4). In place of backwards compatibility, we provide adopters with a migration guide. You can find the migration guide next to the enhancements document.
Created attachment 202047 [details] Patch_v1 This patch all the files inline with our last discussion and i tried to get in the implementation with a minimal chnage to the RootXmlResource. Hope I have the mechanics working now
Created attachment 202048 [details] mylyn/context/zip
i guess your comment#8 corrsed over before I submitted the patch, please have a look at the Factory Class, i just seperated the Controller build to that so as to enable the other forms of grammar later on (if we plan to support) and delgated the entire Controller building stuff to the Factory and RootXmlResource is will do just its job related to resource inits and other stuff. Cool if the backward compatibility is not needed then i shall remove the attributes schema related attributes form the @XmlRootBinding and will update the document for its usage from 0.3 --> 0.4 Also please chnage the satus to assigned as I have already started to work on this.
I am finally remembering all aspects of this API that I've been intending to correct. Let me fill you in on that... @XmlRootBinding predates @XmlNamespace that provides for more flexible way specify namespaces. I've been meaning to clean this up. Might as well do all of this cleanup in one shot. If @XmlNamespace owns association of a prefix with a namespace uri and @XmlSchema owns association of namespace uri with schema location, then there is not much left for @XmlRootBinding (just the element name). Given that, I think we can just drop this annotation and use the existing generic @XmlBinding in its place. Declarations would look something like this: @XmlNamespace( prefix = "m", uri = "http://something.com" ) @XmlSchema( namespace = "http://something.com", location = "http://something.com/schema.xsd" ) @XmlBinding( path = "m:root" ) or @XmlDocType( ... ) @XmlBinding( path = "root" ) or just @XmlBinding( path = "root" ) To complete this change, I'd like to move StandardRootElementController into the internal package. It doesn't make for good API and will need to change extensively anyway for these changes. Does the above make sense? I feel like I am piling all this cleanup on you. If you don't feel comfortable doing it all, we can split this up. We could limit this change to @XmlDocType/DtdRootElementController and I can handle the rest... Misc comments... 1. @XmlDoctype attributes (one or both) should be optional. In some cases both publicId and systemId aren't used. 2. Should it be @XmlDocumentType instead? The term "doctype" seems to have been born of XML syntax. Probably shouldn't enshrine that in our API. 3. @XmlSchema should not have defaultPrefix (that's handled by @XmlNamespace) and schemaLocation attribute should be shortened to just "location". 4. While we may want a factory concept here in the future, I prefer to not add API until we need it. It's hard to know what exactly is needed and more API means more things to support. Let's inline RootElementControllerFactory logic into RootXmlResource. 5. We need to make sure not to forget the case where there is no @XmlSchema or @XmlDocumentType.
(In reply to comment #12) > I am finally remembering all aspects of this API that I've been intending to > correct. Let me fill you in on that... @XmlRootBinding predates @XmlNamespace > that provides for more flexible way specify namespaces. I've been meaning to > clean this up. Might as well do all of this cleanup in one shot. If > @XmlNamespace owns association of a prefix with a namespace uri and @XmlSchema > owns association of namespace uri with schema location, then there is not much > left for @XmlRootBinding (just the element name). Given that, I think we can > just drop this annotation and use the existing generic @XmlBinding in its place. > I was actually thinking of it, and now agree with you as we can just drop the @XmlSchema annotation and continue with the @XmlRootBinding > Declarations would look something like this: > > @XmlNamespace( prefix = "m", uri = "http://something.com" ) > @XmlSchema( namespace = "http://something.com", location = > "http://something.com/schema.xsd" ) > @XmlBinding( path = "m:root" ) > > or > > @XmlDocType( ... ) > @XmlBinding( path = "root" ) > > or just > > @XmlBinding( path = "root" ) > > To complete this change, I'd like to move StandardRootElementController into the > internal package. It doesn't make for good API and will need to change > extensively anyway for these changes. > I can move the StandardRootElementController to the internal package and will do the clean up as part of seperate ticket to track it and focused > Does the above make sense? I feel like I am piling all this cleanup on you. If > you don't feel comfortable doing it all, we can split this up. We could limit > this change to @XmlDocType/DtdRootElementController and I can handle the rest... > I feel lets go Step by Step , first I will push the implementation for this Bug and then we can open a new ticket for the clean up activity as taking too much things at a time will cause commotion and we will go back to Y <<<< X , i want that to reduce this time > Misc comments... > > 1. @XmlDoctype attributes (one or both) should be optional. In some cases both > publicId and systemId aren't used. > Makes sense and updated > 2. Should it be @XmlDocumentType instead? The term "doctype" seems to have been > born of XML syntax. Probably shouldn't enshrine that in our API. > Makes sense and updated > 3. @XmlSchema should not have defaultPrefix (that's handled by @XmlNamespace) > and schemaLocation attribute should be shortened to just "location". > removed, right now @XmlSchema has only namespace and location ( note schemaLocation has been chnaged to location) this change requires us to update the sample projects too , let me know if we can do that as part of this fix ? > 4. While we may want a factory concept here in the future, I prefer to not add > API until we need it. It's hard to know what exactly is needed and more API > means more things to support. Let's inline RootElementControllerFactory logic > into RootXmlResource. > Alright, i have moved the getController rather I call it buildController to the RootXmlResource class > 5. We need to make sure not to forget the case where there is no @XmlSchema or > @XmlDocumentType. I understand, will review all my changes again and may be do a careful study and send you the Patch v2 later today for your review
(In reply to comment #12) > I am finally remembering all aspects of this API that I've been intending to > correct. Let me fill you in on that... @XmlRootBinding predates @XmlNamespace > that provides for more flexible way specify namespaces. I've been meaning to > clean this up. Might as well do all of this cleanup in one shot. If > @XmlNamespace owns association of a prefix with a namespace uri and @XmlSchema > owns association of namespace uri with schema location, then there is not much > left for @XmlRootBinding (just the element name). Given that, I think we can > just drop this annotation and use the existing generic @XmlBinding in its place. I did study impact of dropping @XmlRootBinding annotation and it has some major impact on the following packages org.eclipse.sapphire.modeling.xml - src - org.eclipse.sapphire.modeling.xml org.eclipse.sapphire.samples.architecture - src - org.eclipse.sapphire.samples org.eclipse.sapphire.samples.calendar - src - org.eclipse.sapphire.samples org.eclipse.sapphire.samples.contacts - src - org.eclipse.sapphire.samples org.eclipse.sapphire.samples.ezbug - src - org.eclipse.sapphire.samples org.eclipse.sapphire.samples.gallery - src - org.eclipse.sapphire.samples org.eclipse.sapphire.samples.jee.web - src - org.eclipse.sapphire.samples.jee org.eclipse.sapphire.samples.map - src - org.eclipse.sapphire.samples org.eclipse.sapphire.sdk.extensibility - src - org.eclipse.sapphire.sdk org.eclipse.sapphire.tests.modeling.misc.t0008 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.serialization - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.binding.t0003 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.binding.t0007 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.binding.t0008 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.binding.t0010 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.xsd.t0001 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.xsd.t0002 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.tests.modeling.xml.xsd.t0003 - src - org.eclipse.sapphire.tests org.eclipse.sapphire.ui.def - src - org.eclipse.sapphire.ui org.eclipse.sapphire.ui.diagram.geometry - src - org.eclipse.sapphire.ui org.eclipse.sapphire.ui.form.editors.masterdetails.state - src - org.eclipse.sapphire.ui Not went to detail but just made a quick analysis. Ideally if we dropping the annotation then we need to do an impact analysis and fix the broken links in the above package. I personally feel we need to do that with a new ticket rather than this ticket. Let me know your thoughts
Created attachment 202053 [details] Patch_v1_1 1. Contains the refractoring of StandardRootElementController to internal package 2. Modifications to @XmlBinding to support its annoation at Type level 3. Added new classes, 3.1.DocumentTypeRootElementController - just name consistence with annotation 3.2.Added two new annotations @XmlDocumentType and @XmlSchema 3.3.Renamed the variable defaultPrefix to prefix in StandardRootElementController
Created attachment 202054 [details] mylyn/context/zip
> Not went to detail but just made a quick analysis. Ideally if we dropping the > annotation then we need to do an impact analysis and fix the broken links in > the above package. I personally feel we need to do that with a new ticket > rather than this ticket. I am fine with that.
Reviewing Patch v1.1: 1. DocumentTypeRootElementController should move into the internal package as I don't believe this is supposed to be API. 2. There is an inconsistency between implementation DocumentTypeRootElementController.createRootElement() which seems to assume that systemId is always present and the @XmlDocumentType annotation which makes both attributes optional. 3. If we are going to defer the rest of the cleanup to a separate bug, lets limit this patch to the following files: RootXmlResource, @XmlDocumentType and DocumentTypeRootElementController. The rest of the changes really belong to the broader cleanup. This would also make this patch backwards compatible, with incompatibilities and migration requirements introduced in the second phase.
(In reply to comment #18) > Reviewing Patch v1.1: > > 1. DocumentTypeRootElementController should move into the internal package as I > don't believe this is supposed to be API. > Infact I already did that in my updates which i was doing post sending you the patch. > 2. There is an inconsistency between implementation > DocumentTypeRootElementController.createRootElement() which seems to assume that > systemId is always present and the @XmlDocumentType annotation which makes both > attributes optional. > Yes I notice that, since i was putting the skeleton in place i never looked into that will review that and send it in new patch > 3. If we are going to defer the rest of the cleanup to a separate bug, lets > limit this patch to the following files: RootXmlResource, @XmlDocumentType and > DocumentTypeRootElementController. The rest of the changes really belong to the > broader cleanup. This would also make this patch backwards compatible, with > incompatibilities and migration requirements introduced in the second phase. Yes for the patch for this Bug will be 1.RootXmlResource 2.@XmlDocumentType 3.@XmlDocumentType 4.@XmlSchema 5.@XmlBinding - updated to add additonal @Target to Type 6.StandardRootElementController refractoring to internal package 7.DocumentTypeRootElementController Hope you are fine, let me know otherwise.
Since we are splitting the work in two, I'd rather make the separation cleaner by limiting this patch to RootXmlResource, @XmlDocumentType and DocumentTypeRootElementController. The rest of the changes are really about general cleanup of root xml binding API rather than the specific focus on improving DTD support that is this bug.
Am attaching the patch that has only the DTD changes, please review and if OK close this bug. Once you close it i will take an update from CVS and will work on 355751
Created attachment 202112 [details] Patch (Part 1) v2 this patch contains only four files related to the context of the bug 1.@XmlDocumentType 2.DocumentTypeRootElementController 3.RootXmlResource for adapting to @XmlDocumentType 4.StandardRootElementController for adapting to @XmlDocumentType
Created attachment 202113 [details] mylyn/context/zip
A bug is marked as fixed once code is merged into the source repository. Then it's verified in an official build. Once verified, it is closed.
Fine, then I will wait until all these steps are done and take an update to work on 355751. Let me knwo if you require any action from side during these steps.
After a bit of cleanup, I have released the changes to DocumentTypeRootElementController, RootXmlResource and @XmlDocumentType. The changes to StandardRootElementController would have introduced a backwards incompatibility, which would have triggered need for a migration guide entry at this stage. I think it would be better to handle all of the incompatible changes in one pass as part of the other bug, so I left this one out. The only other semantic change that I made that's worth noting is in handling of the case where systemId is empty and publicId is empty. This strikes me as an error case as I don't know what purpose adding DOCTYPE declaration would serve in that case. I have collapsed this case with the case of @XmlDocumentType annotation not being present. If you know this to be a valid scenario, please elaborate. Here is what's necessary before this issue can be fully resolved: 1. Entry in the enhancement guide. 2. Modify TestXmlDtd0003 which is currently using the style of DTD declaration that we are about to eliminate to use the new annotation. This will satisfy the minimal test requirement for this enhancement, but you are welcome to add additional test cases to cover various systemId/publicId possibilities. 3. DocumentTypeRootElementController.checkRootElement() implementation is only partially complete. In addition to checking the element name, it also needs to check that the document's DOCTYPE matches what's specified in @XmlDocumentType annotation. Since I have already released changes based on Patch v2, this patch will stay put for the IP log (should not be marked as deprecated). The rest of the changes should be made relative to current HEAD and an additional patch attached. Do let me know if anything about is unclear.
thanks for making these changes, shall I update my local source base with your changes ? (In reply to comment #26) > After a bit of cleanup, I have released the changes to > DocumentTypeRootElementController, RootXmlResource and @XmlDocumentType. > > The changes to StandardRootElementController would have introduced a backwards > incompatibility, which would have triggered need for a migration guide entry at > this stage. I think it would be better to handle all of the incompatible changes > in one pass as part of the other bug, so I left this one out. > > The only other semantic change that I made that's worth noting is in handling of > the case where systemId is empty and publicId is empty. This strikes me as an > error case as I don't know what purpose adding DOCTYPE declaration would serve > in that case. I have collapsed this case with the case of @XmlDocumentType > annotation not being present. If you know this to be a valid scenario, please > elaborate. > Yeah I see that unnecessary part, but that was put in place just to make sure i handle all possible cases, I agree with you that it not needed > Here is what's necessary before this issue can be fully resolved: > > 1. Entry in the enhancement guide. > Will work on it, I hope its just for @XmlDocumentType and explain a little bit on what it does. Let me know otherwise > 2. Modify TestXmlDtd0003 which is currently using the style of DTD declaration > that we are about to eliminate to use the new annotation. This will satisfy the > minimal test requirement for this enhancement, but you are welcome to add > additional test cases to cover various systemId/publicId possibilities. > I have already updated the DTD test cases but refrained from releasing it as part of the Patch v2 since we agreed to release only a set of files as per our last discussion. > 3. DocumentTypeRootElementController.checkRootElement() implementation is only > partially complete. In addition to checking the element name, it also needs to > check that the document's DOCTYPE matches what's specified in @XmlDocumentType > annotation. > > Since I have already released changes based on Patch v2, this patch will stay > put for the IP log (should not be marked as deprecated). The rest of the changes > should be made relative to current HEAD and an additional patch attached. > I am not clear on the IPLog stuff, can you please eloborate a bit on that?
I was looking on to the changes that you had made in RootXmlResource, the updates you could have done within the buildController method right ? i just pushed the if...else login to that method to it clear and better readbility to avoid clogging of if..else parts there Is there any specific reason why you removed that method ?
> Will work on it, I hope its just for @XmlDocumentType and explain a little bit > on what it does. Let me know otherwise Yep. That should do it. Explain @XmlDocumentType annotation with an example or two. > I have already updated the DTD test cases but refrained from releasing it as > part of the Patch v2 since we agreed to release only a set of files as per our > last discussion. I was thinking of production files in that discussion. The test case changes should be included here as well. > I am not clear on the IPLog stuff, can you please eloborate a bit on that? Eclipse Foundation takes great care to track all contributions to a project so that adopters can be reasonably sure that no misappropriated IP ends up in the project. For small contributions, patches are flagged with iplog+ flag. At the end of each release, Eclipse Legal queries these and reviews patches via various copyright scanning services (like BlackDuck). Sapphire's IP Log: http://www.eclipse.org/projects/ip_log.php?projectid=technology.sapphire Eclipse Legal Process: http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf The reason I mentioned it here is that Patch v3 should be treated as additive to Patch v2 instead of marking Patch v2 as obsolete (which would prevent it from showing up in the IP log). > I was looking on to the changes that you had made in RootXmlResource, the > updates you could have done within the buildController method right ? i just > pushed the if...else login to that method to it clear and better readbility to > avoid clogging of if..else parts there > > Is there any specific reason why you removed that method ? I generally do not like short single-caller private methods as they make code harder to read. By inlining the buildController method, I halved the number of lines necessary and eliminated re-retrieval of annotations that were already available in the caller, etc.
> shall I update my local source base with your changes ? Yes. Sync to HEAD and do further changes based on that.
Created attachment 202229 [details] Patch (Part 2) v1 This patch includes the updates for docs and test cases. It also includes the addtional updates to the DocumentTypeController.checkRootElement for handling DOCTYPE checking and the RootXmlResource has been updated to hadling the publicId or systemId case - previously it was updated to use DocumentTypeController only if publicId and systemId are not null but DOCTYPE could either have publicId or systemId or both Note: I did not do obslete for the previous patch (v2) and context, not sure whether i need to do based on your last comment adding it to IPLog etc., Please let me know once you have reviewed and any updates needs to be done, if not let me know when shall I sync up my local repository. I hope I have made the your working much easier and avoided unnecessary iterations. though it takes some time for me to understand Eclipse.org process with respec to IPLog et al
Created attachment 202230 [details] mylyn/context/zip
1. Spreading unit tests between test cases 3-5 makes it more difficult to tell what is being tested. This is especially the case since test case 5 uses a completely different test scenario. The other problem with test case 5 is that it uses a remote DTD. This will cause a network hit during the test, making the test case more brittle. Instead of taking the test case changes from this patch, I enhanced existing test case 3 to cover all the necessary cases. 2. When looking at unit tests, I discovered that the scenario of (publicId!=null&&systemId==null) must not be valid. Attempting to specify such a case does not produce a DOCTYPE definition. Therefore, there are really two cases: systemId and publicId/systemId. I have modified the XML binding logic accordingly. 3. There were some logic errors in DocumentTypeRootElementControler.checkRootElement. I fixed those errors and modified the method per #2. Committed the last set of changes. This item can not be considered fixed. Please verify. Once verified, I will mark it as closed.
(In reply to comment #33) > 1. Spreading unit tests between test cases 3-5 makes it more difficult to tell > what is being tested. This is especially the case since test case 5 uses a > completely different test scenario. The other problem with test case 5 is that > it uses a remote DTD. This will cause a network hit during the test, making the > test case more brittle. Instead of taking the test case changes from this patch, > I enhanced existing test case 3 to cover all the necessary cases. > I was thinking about and the my intention was not lookup to external site i was trying build a DTD for that purpose, i just want to demostrate the public and systemId scenario, anyways since uou have updated them I am fine with it > 2. When looking at unit tests, I discovered that the scenario of > (publicId!=null&&systemId==null) must not be valid. Attempting to specify such a > case does not produce a DOCTYPE definition. Therefore, there are really two > cases: systemId and publicId/systemId. I have modified the XML binding logic > accordingly. > From the changes you have made i see that we need systemId, so why can't we make it mandatory attribute in the @XmlNamespace annotation, thereby removing the default value. Which will also allow us to skip the throwing of IllegalStateException in DocumenTypeRootElementController, and if you agree we can knock off the normalizeToNull method too. Let me know your thoughts on the same > 3. There were some logic errors in > DocumentTypeRootElementControler.checkRootElement. I fixed those errors and > modified the method per #2. > > Committed the last set of changes. This item can not be considered fixed. Please > verify. Once verified, I will mark it as closed. I am fine and verified other changes, except my previous comment for #2, once I hear from you for the same I shall go ahead and mark it as verified . Also "This item can not be considered fixed" you mean to say "This item can be considered fixed" ??
I am sorry I see that you have already made that change too. You can ignore my previous comment . I will mark it as verified.
Closing.
