search

getodk / briefcase

ODK Briefcase is a Java application for fetching and pushing forms and their contents. It helps make billions of data points from ODK portable. Contribute and make the world a better place! ✨💼✨
https://docs.getodk.org/briefcase-intro
Other
60 stars 156 forks source link

On export, skip submissions that can't be parsed #884

Closed lognaturel closed 3 years ago

lognaturel commented 3 years ago

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 -errors folder 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.

codecov-io commented 3 years ago

Codecov Report

Merging #884 into master will decrease coverage by 0.01%. The diff coverage is 45.45%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
.../org/opendatakit/briefcase/export/ExportToCsv.java 68.62% <0.00%> (-0.61%) 11.00 <0.00> (ø)
...opendatakit/briefcase/export/SubmissionParser.java 73.40% <50.00%> (+2.27%) 24.00 <2.00> (ø)
...g/opendatakit/briefcase/reused/UncheckedFiles.java 44.53% <0.00%> (-1.69%) 31.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b55a171...308f844. Read the comment docs.

lognaturel commented 3 years ago

@getodk/testers I've rebased on master, please take a look.

kkrawczyk123 commented 3 years ago

@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: Peek 2020-10-28 11-54

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? In the last release, I was using java 11 for building and testing. There are also cases to run briefcase on java 8, 9 and 10 and perform basic actions like pull, push and export.

@getodk-bot unlabel "needs testing" @getodk-bot label "behavior verified"

lognaturel commented 3 years ago

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.

kkrawczyk123 commented 3 years ago

It's a run version issue, jar has been build on java 11.

lognaturel commented 3 years ago

Got it, so I will look into it. It's specifically linked to this PR? And the export just hangs?

lognaturel commented 3 years ago

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.

kkrawczyk123 commented 3 years ago

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.

lognaturel commented 3 years ago

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?

kkrawczyk123 commented 3 years ago

Actually what is even weirder nothing meaningful seems to show in log: briefcase .log.txt I wasn't able to reproduce for Central, Aggregate only.

mmarciniak90 commented 3 years ago

console.txt export

lognaturel commented 3 years ago

@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?

mmarciniak90 commented 3 years ago

the server source makes no difference, right?

That's right. I was able to reproduce it on the Aggregate server and Central, as you can see.

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?

Yes, when I'm using Java 8.

And can you confirm everything works as expected when you use Java 11?

When I use Java 11 I don't see problem where forms aren’t immediately showing up in push or export after a pull.

lognaturel commented 3 years ago

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?

mmarciniak90 commented 3 years ago

And what about on 1.17.4? Do the forms show up immediately? Does export work as expected with Java 11?

When I use Java 11 and Briefcase with these changes,

When I use Java 11 and Briefcase 1.17.4

What if you pull, close Briefcase, reopen Briefcase then try the export?

When I use Java 8 and Briefcase with these changes,

When I use Java 8 and Briefcase 1.17.4

lognaturel commented 3 years ago

Great, so we don't worry about forms showing up in the other tabs since that's an ongoing issue.

closing Briefcase and reopening it doesn't cause that forms are showing up in push or export. Only reloading the storage directory fixes this 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?

mmarciniak90 commented 3 years ago

Yeah!! We have a workaround! :tada: Reopen and reload storage directory cause that export is possible with Java 8. Finally, I have the expected status. Screenshot from 2020-10-29 17-19-58

lognaturel commented 3 years ago

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.

mmarciniak90 commented 3 years ago

I completely agree!