Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Fix #33

Merged
merged 2 commits into from
May 16, 2021
Merged

Bug Fix #33

merged 2 commits into from
May 16, 2021

Conversation

Wael-Fathallah
Copy link

Issue I
In some cases we have the same XSD but with different path presentation, for that reason a new private method created to clean the path for example we have /A/B/../B/C then will transform it to /A/B/C

Issue II
It has been notice that if 2 different XSD that their name has the same ending for example ABCBBB.xsd and BBB.xsd, BBB.xsd will be ignored in schema locations Maps

*In some cases we have the same xsd but with different path presentation
*It been notice that if 2 different XSD that there name has the same
ending one will be ignored
@lcduarte
Copy link
Member

Hello,

Thanks for the pull request, took me a while to check it out.

As for the first issue, all seems fine with your solution.

On the second issue it looks like your changes broke my tests. Would the issue be solved if I added a / to the endsWith invocation on addFileToParse as follows:

public void addFileToParse(String schemaLocation) {
    String fileName = schemaLocation.substring(schemaLocation.lastIndexOf("/")+1);

    if (!schemaLocations.contains(schemaLocation) && schemaLocation.endsWith(".xsd") && schemaLocations.stream().noneMatch(sl -> sl.endsWith("/" + fileName))){
        schemaLocations.add(schemaLocation);
        schemaLocationsMap.put(schemaLocation, currentFile);
    }
}

I think this will solve the second issue and will pass the tests. Give me your opinion

@Wael-Fathallah
Copy link
Author

Hey, Thank you for your reply, yes I think your solution is match better to prevent the 2end issue, but I'm not quite sure if we fail in this situation A/B/C/file.xsd and another file with same name but in different locations A/D/file.xsd

I really appreciate your time, and thank you for this nice project :)

@lcduarte
Copy link
Member

I figured out what the issue was. I was storing full and relative paths to the files on the schemaLocations. By changing this to only use full paths I can change it to something similar to what you suggested:

public void addFileToParse(String schemaLocation) {
    String fullSchemaLocation = currentFile.substring(0, currentFile.lastIndexOf('/') + 1) + schemaLocation;

    if (!schemaLocations.contains(fullSchemaLocation) && fullSchemaLocation.endsWith(".xsd")){
        if (schemaLocation.startsWith("http")){
            schemaLocations.add(schemaLocation);
            schemaLocationsMap.put(schemaLocation, currentFile);
        }
        else {
            schemaLocations.add(fullSchemaLocation);
            schemaLocationsMap.put(fullSchemaLocation, currentFile);
        }
    }
}

By using full paths I don't need to only check the file name, I can check the full path and your second issue should be solved.

* schemaLocationMap use a fullSchemaLocation as key in case is local
file or use url as it is
@Wael-Fathallah
Copy link
Author

Hey, sorry for the delay, so I checked the provided solution, and unfortunately parsing my XSDs leads infinity loop, so i had to do some changes there.

  • use clearPath method in case is not url
  • in case it's url we check if the map contains schemaLocation not fullSchemaLocation as this is what we add to the map if schemaLocation start with http.

@lcduarte lcduarte merged commit caaff20 into xmlet:master May 16, 2021
@lcduarte
Copy link
Member

Looks like I'm a bit rusty regarding this project 😄

I'll merge the pull request and deploy the new version with the bugs fixed.

Thanks for your contribution!

@Wael-Fathallah
Copy link
Author

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants