Bugzilla – Full Text Bug Listing
| Summary: | [xpath2] improvements to computation of typed values of nodes, validated by XML Schema primitive types | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Source Editing | Reporter: | Mukul Gandhi <mukul.gandhi> | ||||||||||
| Component: | wst.xpath | Assignee: | Mukul Gandhi <mukul.gandhi> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | David Carver <d_a_carver> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | david_williams, jesper, thatnitind | ||||||||||
| Version: | 3.2 | ||||||||||||
| Target Milestone: | 3.2.2 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | 280798 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Mukul Gandhi
Committed changes to both HEAD and "R3_2_maintenance" branch. Mukul...for R3_2_Mainteance branches, we need to have approval before committing the changes. Please add a patch with the diff, and add me or jesper for review. I'm fine with the change going into HEAD. Hi Dave, (In reply to comment #2) > Mukul...for R3_2_Mainteance branches, we need to have approval before > committing the changes. Please add a patch with the diff, and add me or jesper > for review. > I'm fine with the change going into HEAD. Could you please clarify, what is "R3_2_maintenance" branch exactly for? How is it different from HEAD? It'll be good for me to know this, for future commits as well :) I'll be attaching the patch, for review shortly. Regards, Mukul (In reply to comment #3) > Hi Dave, > > (In reply to comment #2) > > Mukul...for R3_2_Mainteance branches, we need to have approval before > > committing the changes. Please add a patch with the diff, and add me or jesper > > for review. > > I'm fine with the change going into HEAD. > > Could you please clarify, what is "R3_2_maintenance" branch exactly for? How is > it different from HEAD? > > It'll be good for me to know this, for future commits as well :) > Sure no problem. PsychoPath is part of the WTP 3.2 release that just happened. There will be a WTP 3.2 SR1 in September. The R3_2_maintenance branch is for these service releases, and any change that goes into this branch needs a second commiters approval. R3_2_maintenance needs to stay API compatible so no big refactorings just bug fixes. Any new functionality, refactorings, design changes, etc, go into HEAD. Anything in head should be targetd for 3.3M1, or 3.3 in general. Created attachment 173062 [details]
patch diff file for resolving this bug
Hi Dave, I've reverted the changes to old state (i.e, with deficiency present) on CVS server (for the "R3_2_maintenance" branch). Attached is the patch with the proposed improvements. Kindly review please. Regards, Mukul Thanks, Dave for the details. This helps me. (In reply to comment #4) > Sure no problem. PsychoPath is part of the WTP 3.2 release that just happened. > There will be a WTP 3.2 SR1 in September. The R3_2_maintenance branch is for > these service releases, and any change that goes into this branch needs a > second commiters approval. R3_2_maintenance needs to stay API compatible so no > big refactorings just bug fixes. > Any new functionality, refactorings, design changes, etc, go into HEAD. > Anything in head should be targetd for 3.3M1, or 3.3 in general. (In reply to comment #6) > Hi Dave, > I've reverted the changes to old state (i.e, with deficiency present) on CVS > server (for the "R3_2_maintenance" branch). > > Attached is the patch with the proposed improvements. > > Kindly review please. A couple of refactoring suggestions to help keep the code clean. 1. The inlined strings should really be be extracted to constants. You can use the eclipse refactoring option "Extract Constant" to make this fairly easy. 2. The size of the if statement now is almost screaming for the if to be extracted into a Factory pattern. So instead of having the big long if statement switch to return the correct value within this method, we could extract it to a new Class that created the schemaTypeValue. if ("date".equals(typeName)) { schemaTypeValue = XSDate.parse_date(string_value()); } else if ("anyURI".equals(typeName)) { schemaTypeValue = new XSAnyURI(strValue); } Could simply become: schemaTypeValue = SchemaTypeValueFactory.create(typeName); or SchemaTypeValueFactory factory = new SchemaTypeValueFactory(typeName); schemaTypeValue = factory.newSchemaTypeValue(); The if logic could then be moved to the SchemaTypeValueFactory method. I would still extract the values to constants. Jesper any thoughts on this? Hi Dave, I like the suggestion. Would you like to have this refactoring improvement go into code-base, along with this functional improvement? Or this can wait for the 2nd commit (which will only be a refactoring improvement)? The functional improvement as reflected in this patch is important, because say for example, an XML Schema 1.1 assertions like following: <xs:element name="x" type="xs:positiveInteger" /> <xs:assert test="x mod 2 = 0" /> currently fails due to absence of certain schema types in the code-base, that we are discussing. In case, you would allow to have the patch committed as it is, I promise to make the refactoring improvements asap (say, within a max week's time :( ). Regards, Mukul (In reply to comment #8) > A couple of refactoring suggestions to help keep the code clean. > 1. The inlined strings should really be be extracted to constants. You can use > the eclipse refactoring option "Extract Constant" to make this fairly easy. > 2. The size of the if statement now is almost screaming for the if to be > extracted into a Factory pattern. > So instead of having the big long if statement switch to return the correct > value within this method, we could extract it to a new Class that created the > schemaTypeValue. > if ("date".equals(typeName)) { > schemaTypeValue = XSDate.parse_date(string_value()); > } else if ("anyURI".equals(typeName)) { > schemaTypeValue = new XSAnyURI(strValue); > } > Could simply become: > schemaTypeValue = SchemaTypeValueFactory.create(typeName); > or > SchemaTypeValueFactory factory = new SchemaTypeValueFactory(typeName); > schemaTypeValue = factory.newSchemaTypeValue(); > The if logic could then be moved to the SchemaTypeValueFactory method. > I would still extract the values to constants. > Jesper any thoughts on this? (In reply to comment #9) > Hi Dave, > I like the suggestion. > > Would you like to have this refactoring improvement go into code-base, along > with this functional improvement? Or this can wait for the 2nd commit (which > will only be a refactoring improvement)? > > The functional improvement as reflected in this patch is important, because say > for example, an XML Schema 1.1 assertions like following: > > <xs:element name="x" type="xs:positiveInteger" /> > <xs:assert test="x mod 2 = 0" /> > > currently fails due to absence of certain schema types in the code-base, that > we are discussing. > > In case, you would allow to have the patch committed as it is, I promise to > make the refactoring improvements asap (say, within a max week's time :( ). I'd like the code improvement to go in with this patch as well. Otherwise it'll just get lost. Also make sure you have added tests for all the additional checks you have as well, as I didn't see any tests with this patch. (In reply to comment #10) > I'd like the code improvement to go in with this patch as well. Otherwise > it'll just get lost. Also make sure you have added tests for all the > additional checks you have as well, as I didn't see any tests with this patch. No problems about that. I'll make the changes as you've suggested along with few additional test cases, and submit an updated patch ASAP. Regards, Mukul Created attachment 173198 [details]
updated patch file for the bug
Created attachment 173199 [details]
test case patch file
Hi Dave, I've implemented the improvements as suggested below, and have attached the updated patch along with test-case. Could you please review. Regards, Mukul (In reply to comment #8) > A couple of refactoring suggestions to help keep the code clean. > > 1. The inlined strings should really be be extracted to constants. You can use > the eclipse refactoring option "Extract Constant" to make this fairly easy. > > 2. The size of the if statement now is almost screaming for the if to be > extracted into a Factory pattern. > > So instead of having the big long if statement switch to return the correct > value within this method, we could extract it to a new Class that created the > schemaTypeValue. > > if ("date".equals(typeName)) { > schemaTypeValue = XSDate.parse_date(string_value()); > } else if ("anyURI".equals(typeName)) { > schemaTypeValue = new XSAnyURI(strValue); > } > > Could simply become: > > schemaTypeValue = SchemaTypeValueFactory.create(typeName); > > or > > SchemaTypeValueFactory factory = new SchemaTypeValueFactory(typeName); > schemaTypeValue = factory.newSchemaTypeValue(); > > The if logic could then be moved to the SchemaTypeValueFactory method. > > I would still extract the values to constants. > > Jesper any thoughts on this? (In reply to comment #14) > Hi Dave, > I've implemented the improvements as suggested below, and have attached the > updated patch along with test-case. > > Could you please review. > > Regards, > Mukul > Looks better. One additional suggestion now that we have the Factory, in the method that returns the value I suggest changing the if...else...if to something like: if (XSConstants.ANY_URI.equals(typeName) { return new XSAnyURI(strValue); } if (XSContants.BOOLEAN.equals(typeName)) { return new XSBoolean(Boolean.valueOf(strValue)); } . . . . So basicaly instead of doing if else if since we just return at the end, just return immediately. Makes it a little bit easier to read. Created attachment 173267 [details] updated patch as per comment 15 Hi Dave, Thanks for the suggestion. It's helpful. I've updated the patch as per below comment. Kindly review please. Regards, Mukul (In reply to comment #15) > Looks better. One additional suggestion now that we have the Factory, in the > method that returns the value I suggest changing the if...else...if to > something like: > if (XSConstants.ANY_URI.equals(typeName) { > return new XSAnyURI(strValue); > } > if (XSContants.BOOLEAN.equals(typeName)) { > return new XSBoolean(Boolean.valueOf(strValue)); > } > . > . > . > . > So basicaly instead of doing if else if since we just return at the end, just > return immediately. Makes it a little bit easier to read. Hello Dave & Jesper, I guess for this change to go into service branch now, we need to have a PMC review for this issue. Do you think, this change is worthy for delivery on 2010-07-30 (which is a planned release for 3.2.1) & we must ask for a PMC review? Regards, Mukul I'm applying this issue for PMC review for inclusion into service branch. I think, it's useful to have this change go into the release on 2010-07-30 with WTP 3.2.1. All the changes for this issue are completed after review by Dave Carver (the last comment being "comment 15" which is incorporated). Here are some relevant information about this request: * this is a low risk addition (and very lean) to code-base & is not tightly coupled with other parts of PsychoPath engine code base. But this adds an important improvement to the PsychoPath XPath engine (please read the discussion in this issue, to know what this is about). * How has the fix been tested? It works with all our 8312 tests, and additional tests for this issue. * Give a brief technical overview: PsychoPath engine finds the typed value of XDM nodes, which is available during evaluation of XPath expressions. The current support in PsychoPath code-base was a little incomplete. This patch adds few more XML Schema primitive data-types to this support, making the PsychoPath XPath engine more useful. * Who has reviewed this fix? Dave Carver * What is the risk associated with this fix? None. This is a very lean & safe change to the code-base. Thanks for the consideration. I've commited the changes for this bug to the 3.2.1 service branch, to HEAD and to the PsychoPath test project. If needed for this bug, if somebody could look at the releng configuration, that would be great. I might be a bit slow in doing that at the moment. (In reply to comment #20) > I've commited the changes for this bug to the 3.2.1 service branch, to HEAD and > to the PsychoPath test project. > > If needed for this bug, if somebody could look at the releng configuration, > that would be great. I might be a bit slow in doing that at the moment. Notice that bug 280798 is apparently pulled for 3.2.1 (which is a damn shame), but since it is still in CVS, this bug is blocked as well. I recommend reopening to track this. re-opening per comments. Let's try to get this in early and release in the 3.2.2 cycle so we don't have to go through the pmc approval process. Done, the processor code was already in CVS, the test code is committed now. |