search

opensensorhub / osh-core

OpenSensorHub Core Modules
http://docs.opensensorhub.org
Mozilla Public License 2.0
32 stars 16 forks source link

[CSAPI] Fix exception after parsing procedure XML #252

Closed matthijskooijman closed 1 day ago

matthijskooijman commented 8 months ago

When parsing the XML for a (new) procedure, BaseResourceHandler.create() calls SmlProcessBindingSmlXml.deserialize() repeatedly until there is no more data to be parsed.

However, the code that decides there is no more data did not actually work. After parsing the last object, the parser would still point at the last closing tag, so hasNext() would return true. However, the only event that then is still available is END_OF_DOCUMENT, which is not enough for nextTag(), which then throws.

In practice, this meant that an object to be added would be added as expected but then an exception was raised.

Note that this has not been tested with actually adding more than one object, since I could not figure out how to format multiple objects in a way they would be accepted at all (simply concatenating them produces "Illegal to have multiple roots").

This fixes #251

matthijskooijman commented 8 months ago

Note that I suspect the same problem exists in the SmlFeatureBindingSmlXml class. If this approach is accepted, I can apply the same fix there if needed (though I do not have a setup ready to test that).

matthijskooijman commented 2 days ago

I see this and other PRs are closed without review and without comment, what is the plan here? I spent some time to submit these fixes, which solve actual problems I ran into. It would be nice to see these merged?

alexrobin commented 2 days ago

Hi Matthijs, sorry about that. I didn't mean to close this PR. It was closed automatically when I deleted the v2 branch now that the work has moved to master. I didn't realize GitHub would close PRs based on a deleted branch automatically. Let me try to fix this mess.

alexrobin commented 2 days ago

Alright, I was able to recreate the v2 branch, reopen the PR and then change the base to master so we're good to go now. I should be able to process this in the next few days.

I am going to delete the v2 branch again so please track master from now on.

alexrobin commented 1 day ago

I fixed this slightly differently in 4f5fc2e1 to avoid generating an exception.

matthijskooijman commented 1 day ago

Hi Matthijs, sorry about that. I didn't mean to close this PR. It was closed automatically when I deleted the v2 branch now that the work has moved to master. I didn't realize GitHub would close PRs based on a deleted branch automatically.

Ah, that explains, thanks :-)

I fixed this slightly differently in https://github.com/opensensorhub/osh-core/commit/4f5fc2e1f6df495f3d11034fa9c128abb015dcec to avoid generating an exception.

Cool, that would be even better. But are you sure this works? Could you reproduce the original problem?

Looking at the code and my PR, I'm deducing (it has been a while, so I do not fully remember) that in the problematic case (deserialize() is called a second time, while the parser points at the last closing tag followed by end of document), hasNext() returns true (otherwise the existing !hasNext check would have returned and then next() would raise an exception. With your changes, I think the flow is the same (!hasNext check and then call next()), except when the while condition is false (so parser pointing at START_ELEMENT), since then the entire while is skipped (but I would except the subsequent readDescribedObject to run into an error then). I also recall that the XML parser behaved a bit weird around how it advanced the pointer and dealt with whitespace or so, but I cannot recall how exactly (and maybe the dependency was upgraded since I reported this?).

It's a bit theoretical (It has been a while since I worked with OSH and do not have my test setup ready to go, but I am planning to pick this up again soon), so if you tested this and it works, then I must be misunderstanding :-)

Edit: Nvm, I missed on change, see comment below

matthijskooijman commented 1 day ago

Oh wait, you also switched from calling nextTag() to calling next(), that is the key difference and explains why this works (since next() probably does not throw at the end of the document). This does mean that you might now skip other stuff than just the whitespace nextTag() skips, but I guess that's acceptable (in a well-formed document there is nothing else anyway).

alexrobin commented 1 day ago

Yes, I was able to reproduce the problem and tested that this fix works too. Skipping to the next START_ELEMENT is basically what nextTag() does too but this allows us to detect the end of the document and abort cleanly.