> On 21 Jan 2020, at 00:00, Gary Gregory <garydgreg...@gmail.com> wrote:
>
> On Mon, Jan 20, 2020 at 6:43 PM Rob Tompkins <chtom...@gmail.com
> <mailto:chtom...@gmail.com>> wrote:
>
>>
>>
>>> On Jan 20, 2020, at 6:41 PM, Gary Gregory <garydgreg...@gmail.com>
>> wrote:
>>>
>>> On Mon, Jan 20, 2020 at 6:28 PM Gary Gregory <garydgreg...@gmail.com>
>> wrote:
>>>
>>>>> On Sun, Jan 19, 2020 at 7:39 AM Alex Herbert <alex.d.herb...@gmail.com
>>>
>>>>> wrote:
>>>>>
>>>>> Hi Gary,
>>>>>
>>>>> I raised a few niggles a while back with CSV and the discussion did not
>>>>> receive a response on how to proceed.
>>>>>
>>>>> There is the major bug CSV-248 where the CSVRecord is not Serializable
>>>>> [1]. This requires a decision on what to do to fix it. This bug is
>> still
>>>>> present in 1.8 RC1 as found by FindBugs [2].
>>>>>
>>>>> From what I can see the CSVRecord maintains a reference to the
>> CSVParser.
>>>>> This chain of objects maintained in memory is not serializable and
>> leads
>>>>> back to the original input Reader.
>>>>>
>>>>> I can see from the JApiCmp report that the serial version id was
>> changed
>>>>> for CSVRecord this release so there is still an intention to support
>>>>> serialization. So this should be a blocker.
>>>>>
>>>>> I could not find a serialisation test in the unit tests for CSVRecord.
>>>>> This quick test added to CSVRecordTest fails:
>>>>>
>>>>>
>>>>> @Test
>>>>> public void testSerialization() throws IOException {
>>>>> CSVRecord shortRec;
>>>>> try (final CSVParser parser = CSVParser.parse("a,b",
>>>>> CSVFormat.newFormat(','))) {
>>>>> shortRec = parser.iterator().next();
>>>>> }
>>>>> final ByteArrayOutputStream out = new ByteArrayOutputStream();
>>>>> try (ObjectOutputStream oos = new ObjectOutputStream(out)) {
>>>>> oos.writeObject(shortRec);
>>>>> }
>>>>> }
>>>>>
>>>>> mvn test -Dtest=CSVRecordTest
>>>>>
>>>>> [ERROR] testSerialization Time elapsed: 0.032 s <<< ERROR!
>>>>> java.io.NotSerializableException: org.apache.commons.csv.CSVParser
>>>>> at
>>>>>
>> org.apache.commons.csv.CSVRecordTest.testSerialization(CSVRecordTest.java:235)
>>>>>
>>>>> If I mark the field csvParser as transient it passes. So this is a
>>>>> problem as raised by FindBugs.
>>>>>
>>>>
>>>> Making the field transient would indeed fix this test but... some of the
>>>> record APIs would then fail with NPEs... so we would be kicking the can
>>>> down the road basically. Making the parser serializable is going in the
>>>> wrong direction feature-wise IMO, so let's not go there. A serialization
>>>> proxy would be less worse but should we provide such a thing? I would
>> say
>>>> no. I am OK with making the field transient despite the can kicking, so
>>>> let's do that.
>>>>
>>>
>>> I suppose we should bump the serialVersionUID...
>>
>> +1 to that
>>
>
> The pickle if we do that is that we will be binary incompatible as JApiCmp
> notes:
> MODIFIED (Serializable incompatible(!): serialVersionUID modified) final
> public class org.apache.commons.csv.CSVRecord
If the field is transient then no need to change the serial version ID. That
changes when the classes serialised from an old version cannot be deserialised
with the new version. Since the ‘new’ parser field is transient it should be
ignored.
No one will have a class serialised using version 1.7. It would have thrown an
exception as the parser is not serialisable. So we go back to 1.6 and serialise
a CSVRecord and check it can be deserialised by 1.8.
Alex
>
> Gary
>
>>
>>
>>> Gary
>>>
>>>
>>>>
>>>> Gary
>>>>
>>>>
>>>>>
>>>>>
>>>>> I also raised [3] the strange implementation of the CSVParser
>>>>> getHeaderNames() which ignores null headers as they cannot be used as
>> a key
>>>>> into the map. However the list of column names could contain the null
>>>>> values. This test currently fails:
>>>>>
>>>>> @Test
>>>>> public void testHeaderNamesWithNull() throws IOException {
>>>>> final Reader in = new
>>>>> StringReader("header1,null,header3\n1,2,3\n4,5,6");
>>>>> final Iterator<CSVRecord> records = CSVFormat.DEFAULT.withHeader()
>>>>>
>>>>> .withNullString("null")
>>>>>
>>>>> .withAllowMissingColumnNames()
>>>>>
>>>>> .parse(in).iterator();
>>>>> final CSVRecord record = records.next();
>>>>> assertEquals(Arrays.asList("header1", null, "header3"),
>>>>> record.getParser().getHeaderNames());
>>>>> }
>>>>>
>>>>> I am not saying it should pass but at least the documentation should
>>>>> state the behaviour in this edge case. That is the list of header
>> names may
>>>>> be shorter than the number of columns when the parser is configured to
>>>>> allow null headers. I’ve not raised a bug ticket for this as it is
>> open to
>>>>> opinion if this is by design or actually a bug. This issue is still
>> present
>>>>> in 1.8 RC1.
>>>>>
>>>>> Previously I suggested documentation changes for this and another edge
>>>>> case using the header map to be added to the javadoc for
>> getHeaderNames()
>>>>> and getHeaderMap():
>>>>>
>>>>> - Documentation:
>>>>>
>>>>> The mapping is only guaranteed to be a one-to-one mapping if the record
>>>>> was created with a format that does not allow duplicate or null header
>>>>> names. Null headers are excluded from the map and duplicates can only
>> map
>>>>> to 1 column.
>>>>>
>>>>>
>>>>> - Bug / Documentation
>>>>>
>>>>> The CSVParser only stores headers names in a list of header names if
>> they
>>>>> are not null. So the list can be shorter than the number of columns if
>> you
>>>>> use a format that allows empty headers and contains null column names.
>>>>>
>>>>>
>>>>> The ultimate result is that we should document that the purpose of the
>>>>> header names is to provide a list of non-null header names in the order
>>>>> they occur in the header and thus represent keys that can be used in
>> the
>>>>> header map. In certain circumstances there may be more columns in the
>> data
>>>>> than there are header names.
>>>>>
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>> [1] https://issues.apache.org/jira/browse/CSV-248 <
>>>>> https://issues.apache.org/jira/browse/CSV-248>
>>>>>
>>>>> [2]
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/findbugs.html
>>>>> <
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/findbugs.html
>>>>>>
>>>>>
>>>>> [3] https://markmail.org/message/woti2iymecosihx6 <
>>>>> https://markmail.org/message/woti2iymecosihx6>
>>>>>
>>>>>
>>>>>
>>>>>> On 18 Jan 2020, at 17:52, Gary Gregory <ggreg...@apache.org> wrote:
>>>>>>
>>>>>> We have fixed quite a few bugs and added some significant enhancements
>>>>>> since Apache Commons CSV 1.7 was released, so I would like to release
>>>>>> Apache Commons CSV 1.8.
>>>>>>
>>>>>> Apache Commons CSV 1.8 RC1 is available for review here:
>>>>>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1 (svn
>>>>>> revision 37670)
>>>>>>
>>>>>> The Git tag commons-csv-1.8-RC1 commit for this RC is
>>>>>> c1c8b32809df295423fc897eae0e8b22bfadfe27 which you can browse here:
>>>>>>
>>>>>>
>>>>>
>> https://gitbox.apache.org/repos/asf?p=commons-csv.git;a=commit;h=c1c8b32809df295423fc897eae0e8b22bfadfe27
>>>>>> You may checkout this tag using:
>>>>>> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
>>>>> --branch
>>>>>> commons-csv-1.8-RC1 commons-csv-1.8-RC1
>>>>>>
>>>>>> Maven artifacts are here:
>>>>>>
>>>>>>
>>>>>
>> https://repository.apache.org/content/repositories/orgapachecommons-1489/org/apache/commons/commons-csv/1.8/
>>>>>>
>>>>>> These are the artifacts and their hashes:
>>>>>>
>>>>>> #Release SHA-512s
>>>>>> #Sat Jan 18 12:01:01 EST 2020
>>>>>>
>>>>>
>> commons-csv-1.8-bin.tar.gz=85a876b41aa9ce61f7f533c46df48754e05bddbdef892aed2bac7674b5ea13855de25576364649048dbb55e7fb18a354305b56cb697e85df68a87113954128ed
>>>>>>
>>>>>
>> commons-csv-1.8-bin.zip=9b86a22367c84a0c96a457e8495f81113b64ae5501eabbe2ea4137654b6baa05bcc24a19626452b80e30ff2dd39214840c6ec534be1db9eec2d12c93eeab2de1
>>>>>>
>>>>>
>> commons-csv-1.8-javadoc.jar=a481149dfeffe4e915d5d2e846831994223fc7d09ed2b61398c68eed5a672654a141fa6de705aa743d0b5af6fd24a3f4b0d5e7cee238a1f7642673288d4a985d
>>>>>>
>>>>>
>> commons-csv-1.8-sources.jar=f68e50f8a025a8b2a570b46905b22b5753a83c19bee5c38103d92ec1e47b4e0d27353e7931961e74fe8e67c4909b0ece6ede49a585d2f9180a7a15458b020bc0
>>>>>>
>>>>>
>> commons-csv-1.8-src.tar.gz=c3268f456978e75c19134e35d05bff77002b2fa7439be2623d58a102cab4f93b0913a1a789f962aafcd677938be1547f47c5dd86e3ea08b7bf8f0420e81beb7a
>>>>>>
>>>>>
>> commons-csv-1.8-src.zip=ebb32f2406b6afa48472283612c7d0a94f932d7ae7a72ad1d239e2249de12f1e0da7f61d34d95d66b1d1fe95b66b6316af9d1fc93734f610cce4a7163b0900d0
>>>>>>
>>>>>
>> commons-csv-1.8-test-sources.jar=13508d417e23a5d3f575a39b3aedb20d0d834335d7994f3045fff316e6b12e50cbf9afe908271357b4091d981c178a28dc61bcdb8db60bd0ada07d3de59eacbf
>>>>>>
>>>>>
>> commons-csv-1.8-tests.jar=901889d4be203c2044df89b7e051d21e7b806e5e56438bf9a7483b334331da94b71de1a129c8bf7967e02479a0922bb834ce37eaabf6662702e147813ecb2b7f
>>>>>>
>>>>>> I have tested this with ' mvn -V -Prelease -Ptest-deploy -P jacoco -P
>>>>>> japicmp clean package site deploy' using:
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 1.8.0_241, vendor: Oracle Corporation, runtime:
>> C:\Program
>>>>>> Files\Java\jdk1.8.0_241\jre
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Additional tests with 'mvn -V clean test' using:
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 1.8.0_241, vendor: Oracle Corporation, runtime:
>> C:\Program
>>>>>> Files\Java\jdk1.8.0_241\jre
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 11.0.6, vendor: Oracle Corporation, runtime: C:\Program
>>>>>> Files\Java\jdk-11.0.6
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 12.0.2, vendor: Oracle Corporation, runtime: C:\Program
>>>>>> Files\Java\jdk-12.0.2
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 13.0.2, vendor: Oracle Corporation, runtime: C:\Program
>>>>>> Files\Java\jdk-13.0.2
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 14-ea, vendor: Oracle Corporation, runtime: C:\Program
>>>>>> Files\Java\openjdk\jdk-14
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> JaCoCo fails on Java 15-EA because it does not know about class file
>>>>> major
>>>>>> version 59:
>>>>>>
>>>>>> Caused by: java.lang.IllegalArgumentException: Unsupported class file
>>>>> major
>>>>>> version 59
>>>>>> at
>>>>>>
>>>>>
>> org.jacoco.agent.rt.internal_43f5073.asm.ClassReader.<init>(ClassReader.java:195)
>>>>>>
>>>>>> Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
>>>>>> Maven home: C:\Java\apache-maven-3.6.3\bin\..
>>>>>> Java version: 15-ea, vendor: Oracle Corporation, runtime: C:\Program
>>>>>> Files\Java\openjdk\jdk-15
>>>>>> Default locale: en_US, platform encoding: Cp1252
>>>>>> OS name: "windows 10", version: "10.0", arch: "amd64", family:
>> "windows"
>>>>>>
>>>>>> Details of changes since 1.7 are in the release notes:
>>>>>>
>>>>>>
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/RELEASE-NOTES.txt
>>>>>>
>>>>>>
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/changes-report.html
>>>>>>
>>>>>> Site:
>>>>>>
>>>>>>
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/index.html
>>>>>> (note some *relative* links are broken and the 1.8 directories are
>>>>> not
>>>>>> yet created - these will be OK once the site is deployed.)
>>>>>>
>>>>>> JApiCmp Report (compared to 1.7):
>>>>>>
>>>>>>
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/japicmp.html
>>>>>>
>>>>>> RAT Report:
>>>>>>
>>>>>>
>>>>>
>> https://dist.apache.org/repos/dist/dev/commons/csv/1.8-RC1/site/rat-report.html
>>>>>>
>>>>>> KEYS:
>>>>>> https://www.apache.org/dist/commons/KEYS
>>>>>>
>>>>>> Please review the release candidate and vote.
>>>>>> This vote will close no sooner that 72 hours from now.
>>>>>>
>>>>>> [ ] +1 Release these artifacts
>>>>>> [ ] +0 OK, but...
>>>>>> [ ] -0 OK, but really should fix...
>>>>>> [ ] -1 I oppose this release because...
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Gary Gregory,
>>>>>> Release Manager (using key 86fdc7e2a11262cb)
>>>>>>
>>>>>> For following is intended as a helper and refresher for reviewers.
>>>>>>
>>>>>> Validating a release candidate
>>>>>> ==============================
>>>>>>
>>>>>> These guidelines are NOT complete.
>>>>>>
>>>>>> Requirements: Git, Java, Maven.
>>>>>>
>>>>>> You can validate a release from a release candidate (RC) tag as
>> follows.
>>>>>>
>>>>>> 1) Clone and checkout the RC tag
>>>>>>
>>>>>> git clone https://gitbox.apache.org/repos/asf/commons-csv.git
>> --branch
>>>>>> commons-csv-1.8-RC1 commons-csv-1.8-RC1
>>>>>> cd commons-csv-1.8-RC1
>>>>>>
>>>>>> 2) Check Apache licenses
>>>>>>
>>>>>> This step is not required if the site includes a RAT report page which
>>>>> you
>>>>>> then must check.
>>>>>>
>>>>>> mvn apache-rat:check
>>>>>>
>>>>>> 3) Check binary compatibility
>>>>>>
>>>>>> Older components still use Apache Clirr:
>>>>>>
>>>>>> This step is not required if the site includes a Clirr report page
>> which
>>>>>> you then must check.
>>>>>>
>>>>>> mvn clirr:check
>>>>>>
>>>>>> Newer components use JApiCmp with the japicmp Maven Profile:
>>>>>>
>>>>>> This step is not required if the site includes a JApiCmp report page
>>>>> which
>>>>>> you then must check.
>>>>>>
>>>>>> mvn install -DskipTests -P japicmp japicmp:cmp
>>>>>>
>>>>>> 4) Build the package
>>>>>>
>>>>>> mvn -V clean package
>>>>>>
>>>>>> You can record the Maven and Java version produced by -V in your VOTE
>>>>> reply.
>>>>>> To gather OS information from a command line:
>>>>>> Windows: ver
>>>>>> Linux: uname -a
>>>>>>
>>>>>> 5) Build the site for a single module project
>>>>>>
>>>>>> Note: Some plugins require the components to be installed instead of
>>>>>> packaged.
>>>>>>
>>>>>> mvn site
>>>>>> Check the site reports in:
>>>>>> - Windows: target\site\index.html
>>>>>> - Linux: target/site/index.html
>>>>>>
>>>>>> 6) Build the site for a multi-module project
>>>>>>
>>>>>> mvn site
>>>>>> mvn site:stage
>>>>>> Check the site reports in:
>>>>>> - Windows: target\site\index.html
>>>>>> - Linux: target/site/index.html
>>>>>>
>>>>>> -the end-
>>>>>
>>>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
