Replace univocity-parsers with FastCSV

[vdmitrienko]

Overview

#4339

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in Release Notes

[marcphilipp]

This looks very promising! 👍

[marcphilipp]

Does auto-detection work in all cases? What happens if \n is used in a cell like in the following example with 4 columns?

a;b;\n
c;d\r\n
e;f;g;h\r\n

(assuming \n and \r are replaced with the corresponding character)

[vdmitrienko]

Does auto-detection work in all cases?

The auto-detection treats each of \r, \n, and \r\n as a line separator. For example, given the following input:

a;b\r
c;d\n
e;f\r\n
g;h

The result is:

[["a", "b"], ["c", "d"], ["e", "f"], ["g", "h"]]

In contrast, univocity-parsers (when configured with \n as the line separator) produces different results:

["a", "b\rc", "d"], ["e", "f"], ["g", "h"]

[vdmitrienko]

What happens if \n is used in a cell like in the following example with 4 columns?

In this case, the results from FastCSV and univocity-parsers are mostly similar.

FastCSV:

[["a", "b", null], ["c", "d"], ["e", "f", "g", "h"]]

univocity-parsers:

// .lineSeparator("\n") - same as FastCSV
[["a", "b", null], ["c", "d"], ["e", "f", "g", "h"]]

// .lineSeparator("\r\n") - same as FastCSV
[["a", "b", null], ["c", "d"], ["e", "f", "g", "h"]]

// .lineSeparator("\r") - differs from FastCSV
[["a", "b", "c", "d"], ["e", "f", "g", "h"], [null]]

[osiegmar]

I’m afraid this breaks compatibility if someone uses a character sequence as a line delimiter that is not a newline.

[vdmitrienko]

So, considering 3 possible scenarios, all of them imply a breaking change 😞

1. User explicitly relies on \r\n as the line separator:
   \r - causes an unexpected line break;
   \n - causes an unexpected line break;

2. User explicitly relies on \r as the line separator:
   \n - causes an unexpected line break;
   \r\n - no change, since \r is already interpreted as a line break;

3. User explicitly relies on \n as the line separator:
   \r - causes an unexpected line break;
   \r\n - no change, since \n is already interpreted as a line break;

[vdmitrienko]

@osiegmar, would it be possible to add support for a lineSeparator() parameter in FastCSV?

[osiegmar]

Potentially, yes. Of course, this wouldn’t be a valid CSV file at all. Is this really a desired feature or just lack of specification/documentation and a good chance to change that with the new major version of JUnit?

Is there a (good) reason, someone separates text records by anything that is not a newline sequence?

Is there known usage of this?

[marcphilipp]

I think dropping this in the new major version makes sense. I'm not aware of any cases other than using the same line separator on different operating systems. IIRC I initially introduced it because univocity-parsers would use the system line separator (and only that) be default.

[vdmitrienko]

I think dropping this in the new major version makes sense.

Great 👍
I'll make sure to clarify that in the release notes. Adding a few tests wouldn't hurt either.

[marcphilipp]

Does FastCSV provide an API for line-by-line reading so we don't have to create a string first? It's probably not a big deal since it comes from literals in an annotation.

[osiegmar]

With osiegmar/FastCSV@1077389 there is one now. @vdmitrienko You may want to give it a try if it simplify things for you.

[vdmitrienko]

Thanks, @osiegmar. This works well with individual strings, but it doesn't support headers. I think adding an overload that accepts an array (or varargs) of strings could simplify this use case:

build(final CsvCallbackHandler<T> callbackHandler, final String... data)

[vdmitrienko]

Regarding the validation of empty records, having a setting for that could be quite handy. It would also be great if the exception message included the index of the empty record. That said, we could also handle this on our side 🙂