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

Allow filling in a default "creator" value to work around invalid GPX files from legacy IHM #23

Closed
HarelM opened this issue Sep 4, 2018 · 10 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@HarelM
Copy link
Contributor

HarelM commented Sep 4, 2018

The world isn't perfect :-)
Following on some user feedback I got after publishing this to production.
The following error is a bit cryptic assuming the GPX version is 1.1.

if (version != "1.1" || creator is null)

You'll be able to get some examples from users in the issue they opened:
IsraelHikingMap/Site#794

The problem is that creator is empty in these files.
I'm not sure how they got these files, but preventing opening a file because the creator field does not exist seems too rigid apparently...

@airbreather
Copy link
Member

Wouldn't XSD validation fail for such files?

@HarelM
Copy link
Contributor Author

HarelM commented Sep 4, 2018

It might be that .Net conversion from xsd to cs class doesn't obey the required? I'm not sure...
This is the gpx.cs generated from the xsd:
https://github.com/IsraelHikingMap/Site/blob/ac61da34ca613208d1fb76d07338ca0cbcdaf7fe/IsraelHiking.API/Gpx/gpx.cs

@HarelM
Copy link
Contributor Author

HarelM commented Sep 4, 2018

Well, seems like I was naive to think that a generated class would be used to validate an xml :-)
I didn't have any checks on the GPX file on my site besides XmlDeserialize so it either worked and got the right data or failed while deserializing.
Having said that, I don't know if the fact that the GPX file doesn't have a creator is interesting to the end user.
Can Read accept a "don't throw" flag to workaround this? what do you think? should I send the users to the site they got the GPX from?

@airbreather
Copy link
Member

airbreather commented Sep 4, 2018

Having said that, I don't know if the fact that the GPX file doesn't have a creator is interesting to the end user.
Can Read accept a "don't throw" flag to workaround this? what do you think? should I send the users to the site they got the GPX from?

The GPX schema documentation has this to say about creator being required:

You must include the name or URL of the software that created your GPX document. This allows others to inform the creator of a GPX instance document that fails to validate.

I can improve the error message to help figure out what's wrong when there is some sort of a failure, but I do want this to throw XmlException, by default, in most or all cases that would cause a schema validation failure.

Perhaps GpxReaderSettings could have some flags to let you fill in a default creator if it turns out that these are being produced by an important non-compliant provider, but I'd appreciate it if we could try to figure out who's producing these invalid GPX files first (edit: and try to fix them through other means than working around it at this step).

@HarelM
Copy link
Contributor Author

HarelM commented Sep 4, 2018

I can improve the error message to help figure out what's wrong

This would help, of course.

I have asked the users who opened the issue where they got the files from, I hope they'll respond, and I hope someone at the other end is willing to change their code.
Having said that, I tend to agree that GpxReaderSettings is an appropriate location to place this default value.
While I don't like the fact that someone is generating an invalid GPX file, I can't tell my user to f$@k off, right? :-)

@airbreather
Copy link
Member

airbreather commented Sep 4, 2018

While I don't like the fact that someone is generating an invalid GPX file, I can't tell my user to f$@k off, right? :-)

No, but there's a limit to how much hand-holding makes sense before the answer has to be "fix your data before uploading it". In those cases (and this one, at least for now), one thing you can do is catch specifically XmlException and have the handler run XSD validation to present the errors to the user so that you can show the full list of errors at once.

@HarelM
Copy link
Contributor Author

HarelM commented Sep 4, 2018

Well, seems I can only blame myself - the files where created by my code :-((( in a special scenario I forgot to add creator to the file.
Migrating to this library automatically fixes it, but I'll need to find a way to allow the user to open these damaged files... :-(

@airbreather airbreather self-assigned this Sep 5, 2018
@airbreather airbreather added the enhancement New feature or request label Sep 5, 2018
@airbreather airbreather added this to the Alpha 0.3.0 milestone Sep 5, 2018
@airbreather airbreather changed the title Problem with GPX files without creator Allow filling in a default "creator" value to work around legacy IHM bug Sep 6, 2018
@airbreather airbreather changed the title Allow filling in a default "creator" value to work around legacy IHM bug Allow filling in a default "creator" value to work around invalid GPX files from legacy IHM Sep 6, 2018
@HarelM
Copy link
Contributor Author

HarelM commented Sep 8, 2018

Thanks! let me know when a new package is available.
Sorry for the trouble...

@airbreather
Copy link
Member

let me know when a new package is available.

@HarelM 0.3.0 is up on NuGet now.

@HarelM
Copy link
Contributor Author

HarelM commented Sep 10, 2018

Thanks a lot for all the hard work! keep it up. I have published this change to my production site.

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

No branches or pull requests

2 participants