On export, skip submissions that can't be parsed #884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #884 +/- ##
============================================
- Coverage 48.33% 48.32% -0.02%
+ Complexity 1647 1646 -1
============================================
Files 195 195
Lines 10422 10418 -4
Branches 753 752 -1
============================================
- Hits 5037 5034 -3
+ Misses 5026 5024 -2
- Partials 359 360 +1
Continue to review full report at Codecov.
|
It was previously impossible to track down empty submission files.
|
@getodk/testers I've rebased on master, please take a look. |
|
@lognaturel After long investigation we finally figure out the problem and agreed that the export crash issue is visible on PR's build only on java 8. I was also able to reproduce it (I am normally on java 11 this is why it wasn't visible for me previously), see the gif: The issue is not visible on java 9, 10 and 11. I am not sure which one is the most frequent for users. As the issue is visible only on the old java version it shouldn't block this or. It has been verified on Ubuntu and MacOS. Here comes a question on which java version we should do regression pass? @getodk-bot unlabel "needs testing" |
|
We build releases with Java 11 and we don't typically support users making their own builds. So if it's the build version that makes a difference, I don't think it matters. If it's the run version that makes a difference, then I think we should look into it more deeply. Java 11 from https://adoptopenjdk.net/ is what we recommend in the docs for running Briefcase so let's QA with that. Still, it should run with Java 8. |
|
It's a run version issue, jar has been build on java 11. |
|
Got it, so I will look into it. It's specifically linked to this PR? And the export just hangs? |
|
I'll merge and then investigate on master. I'm guessing that somewhere there's an API not available on Java 8 that's being used but I don't think it's in this PR specifically. |
|
It works better on 1.17.4 where export is starting, % progress is displayed and csv is created but it's empty and shows exactly the issue that has been fixed here. |
|
I just merged, pulled from master, built with java 11, ran with java oracle64-1.8.0.202, and everything worked as expected. If you're seeing a crash, do you get a stack trace on the console? |
|
Actually what is even weirder nothing meaningful seems to show in log: briefcase .log.txt |
|
@mmarciniak90 the server source makes no difference, right? You have that strange thing where forms aren’t immediately showing up in push or export after a pull. Is that also the case on v1.17.4? And can you confirm everything works as expected when you use Java 11? |
That's right. I was able to reproduce it on the Aggregate server and Central, as you can see.
Yes, when I'm using Java 8.
When I use Java 11 I don't see problem where forms aren’t immediately showing up in push or export after a pull. |
Great. And what about on 1.17.4? Do the forms show up immediately? Does export work as expected with Java 11? Another thought for Java 8. What if you pull, close Briefcase, reopen Briefcase then try the export? |
When I use Java 11 and Briefcase with these changes,
When I use Java 11 and Briefcase 1.17.4
When I use Java 8 and Briefcase with these changes,
When I use Java 8 and Briefcase 1.17.4
|
|
Great, so we don't worry about forms showing up in the other tabs since that's an ongoing issue.
And when you reload after re-opening on this PR, does the export work or does it still hang/crash? Does it hang for all forms or only forms with empty submissions? |
|
Yeah!! We have a workaround! 🎉 |
|
Phew! Ok, so the problem you experience is that on Java 8, if there is an empty submission, export fails unless Briefcase is closed and reopened? Export works in other cases, right? I think that means we're no worse off than with v1.17.4 so we can file this as an issue and deal with it if users run into trouble. |
|
I completely agree! |
Closes #874
What has been done to verify that this works as intended?
Tried different exports with and without empty submission.xml files. Clearing the contents of a submission.xml file reproduces the original issue. With this PR, the export should be successful, there should be a message in the details saying
Can't export submission ..., the-errorsfolder should include the failed submission with its instanceID.Why is this the best possible solution? Were any other approaches considered?
This is the only solution I considered once I realized that submissions that were leading to parse errors were included in the submissions to export. This is the simplest way to exclude those submissions.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Users should get fewer export errors and should have more information to recover from them when they do happen. The regression risk is low because it only affects the path where there's an exception on parse.
Does this change require updates to documentation? If so, please file an issue at https://github.com/getodk/docs/issues/new and include the link below.
No.