wrong element is used if a ref="elementName" is not unique#83
Conversation
To fix issue 80, it is not necessary to check for min/maxOccurs. The underlying issue is the duplicate objects in the concreteElementsMap. These duplicate objects are the result of initialising the includedElements list.
When an element is referenced using `ref`, no distinction is made as to whether it is a child of a schema or of another type. All implementations of `clone` write directly to the `placeHolderAttributes` passed to the method. To prevent the caller’s own `placeHolderAttributes` from being altered, `clone` creates a copy of the argument passed to it.
If an http(s) URL is specified in an include, it does not need to be
‘normalised’ in the same way as a file.
on Linux "new File("/a/folder", "http://a.url/issue_25_amzn-base.xsd");"
doesn't throw a IOException. An attempt was therefore made to normalise
the file "a/folder/http://a.url/issue_25_amzn-base.xsd" which of course
did not work.
| XsdParser xsdParser = new XsdParser(getFilePath("issue_83/element_name_not_uq.xsd")); | ||
| XsdSchema xsdSchema = xsdParser.getResultXsdSchemas().findFirst().get(); | ||
|
|
||
| XsdElement rootElement = elementByName(xsdSchema.getChildrenElements(), "rootElemenet", XsdElement.class); |
There was a problem hiding this comment.
typo in rootElemenet
| return Objects.equals(elementMinOccurs, referenceMinOccurs) && Objects.equals(elementMaxOccurs, referenceMaxOccurs); | ||
| } | ||
| private static boolean isNotNested(NamedConcreteElement e) { | ||
| return e.getElement().getParent() != null && e.getElement().getParent() instanceof XsdSchema; |
There was a problem hiding this comment.
unnecessary null-check, e.getElement().getParent() instanceof XsdSchema; will just return false when e.getElement().getParent() is null
|
Hi, I saw you mentioned me. From the unit test you added, I see you found a bug that was still present after I made some fixes. In your test I see there are two separate element definitions. They have the same name, but one is a root element and the other is a nested element. Still their documentation element got mixed up. If the existing unit tests on issues 79 and 80 still pass then I think you made a good improvement. I added two comments but don't feel comfortable enough with the code to do a full review. Hopefully @lcduarte can do this? |
|
Hello Thank you both of your input. I merged this PR to a test branch to try it out but it seems that the |
|
Hello, Thank you very much for your quick replies. I’m sorry – I was so focused on the IssueTest that I overlooked the other tests. That wasn’t my intention. I’ve had a quick look at them, but I now need to rethink my strategy for finding the right elements. I’ll be in touch again. |
|
@lcduarte, how should I now push the fix? I try to push on the |
|
@waldi5001 I don't really know, I'm not very used to github, maybe create a new PR with the additional fixes to that branch? |
|
follow up #84 |
|
If you want one complete PR with nice commits, you can choose #85, that would be my prefered way. But... its up to you. Dying in beauty 🗡️ |
3 participants
Whilst investigating my issue, I came across issue_80 / #79 and realised that the author @HennoVermeulen had likely made a mistake. He had written a workaround for the issue of duplicate objects. I fixed this problem in my first commit.
In my actual commit, I noticed that every call to
clonemodifies theoldElementAttributes. If there were still duplicate objects from the previous commit, this would cause side effects that I encountered whilst investigating this commit. Hence the commit before this. To prevent this problem in principle, I allways create a copy of the map in XsdAbstractElementFinally, I noticed that all the tests run perfectly on Windows, but not on Linux. Specifically: testIssue26_Includes and testIssue25AutoAccessory. I ‘fixed’ this problem quite easily.
Perhaps it would be a good idea to use only URLs internally. I’ve already started doing this, but haven’t finished it yet. It makes handling ‘locations’ much easier.
Let me know if there's anything else I can do.