Source Merge Request #29: Added the option of use number formatters at ResultSetHelperService. (rejected)

WHY: I have the need of formatting the numbers on the CSV output, mostly because of the distinct locale decimal point and miles separator symbols for several locales.
WHAT: I modified the ResultSetHelperService so you can define two number formatters: one for integer numbers and one for non integer (as floating point) numbers. If none of these is defined, the behaviour of the service remains the same it is now.
COMMENTS: In the way of applying this enhancement, I found what I believe is a bug causing that null numbers are rendered as 0, I also fixed it. Also I have had to change the way the resultset is mocked at test as the behaviour of the wasNull function was not consistent and made some tests to fail. I didn't find a way of doing it using mockito, so I used a simple wrapper with method call delegation.

[Ivan Lorca]

Sorry, I was wrong about the bug, my bad. The rest still applies.

[Andrew Rucker Jones]

I like this change, but I have a few questions:
1. If you say you fixed a bug, but then you say you were wrong about the bug, do you need to update this pull request?
2. Why use Generics in the two versions of applyFormatter()? It seems to me Number ought to work fine, unless I'm missing something.
3. I'm a little dubious about your claim that wasNull() behaves inconsistently. Could you walk me through why that change to the test code was necessary?

[Ivan Lorca]

Hello Andew, thanks for your feedback.

1. I Missed the if (rs.wasNull() || value == null) { ... } at the end of function getColumnValue, so I thought it was not being checked, so the code as it was would work ok, it is just checking 2 times if the value is null. I made a bit of cleaning to avoid this in today's commit. While I was at it, I also added a protected method handleBoolean to allow boolean conversion personalization by extending the ResultSetHelperService class, using the same approach already used for other types.
2. Good point! I tried it, but it appears that Number does not have the toBigInteger() function, so it doesn't compile for the Types.BIGINT case, as it is written, so in order to avoid more changes, I kept the generics.
3. If you look at the setExpectToGetColumnValue function at MockResultSetBuilder class, you can see it only populates the null values list (wnrl) for certain types. This list is later used to set the result of the calls to wasNull mocked function. It also relies on that wasNull() is called after every getXxxx() call, and just one time. I think my approach is more consistent as it doesn't care how many (if any) times the wasNull function is called, always returns what it should. I understand that the trade off is adding more complexity to the mock, so I also prepared another version with just the few changes needed for the tests to pass, you can check it at https://sourceforge.net/u/speevy/opencsv/ci/f715cf62a0e99aa69e90c5692607e465ddc74502/

[Andrew Rucker Jones]

I took a long, hard look at this today and made some changes before I committed:
1. You changed am expected test output for your proposed handleBoolean() method. Our project is quite strict that changing tests is not okay. Our extensive test suite is a contract with our users for how opencsv works. Changing a test means we broke backward compatability, and we only do that on major releases and for a good reason. So, I removed the handleBoolean() method and reverted the test change.
2. I am equally concerned about the changes to ResultSetHelperServiceTest for the same reason, but I don't understand Mockito at all, so I can't evaluate how your changes effect the overall test suite. As far as I can tell, all inputs and outputs are unchanged, and that's the key part.
3. But, your addition of wnrl.add(value == null); to the end of MockResultSetBuilder.setExpectToGetColumnValue() is not showing up in the code I fetched. I'm probably doing something wrong, but everything else seems to be there, and I don't know why git would fetch everything except one line.
4. I'm also terribly worried about the BigInteger case, which, from the former code in ResultSetHelperService should have returned the string "null" on null input, yet the unaltered tests indicate it returned an empty string. I can't explain that. I'm guessing it's connected to your changes to the Mockito code.
5. I removed the Optionals around the NumberFormats because they serve no purpose. The code is the exact same without them: you assign, you check for existence, you use.
6. Given the point under #4, it turns out the special code for the BIGINT case in ResultSetHelperService.getColumnValue() was unnecessary. As a result, your two applyFormatter() methods condensed down to one without generics.

Please take a look and let me know what you think.

[Ivan Lorca]

Hello Andrew,

I'm afraid this is taking more trouble than it was intended ... I'm sorry for that.

Also, for some reason, I cannot see you commit on the merge request (in fact I neither can see mine anymore), I'll check again later, and complete my answer as needed.

I understand your concerns, but as for my understanding it all ends on the inconsistencies with the mock for the wasNull method, as mentioned earlier. I explain: I made the change in the boolean test because it was not reflecting the reality of the behaviour of the code, in fact, for this test the mock of wasNull is returning false when it should return true. I think this may be also the case for the BigInt type. Also keep in mind that at the end of the getColumnValue function there is a check for null values and returns an empty string. As for my understanding this is the common behaviour for all column types.

Please notice that the change on the boolean test had nothing to do with the extraction of the boolean conversion code to the handleBoolean function, you can check that if you keep the changes in the resultset mock the test will fail with or without this change.

For points 5 and 6 It's ok for me. I prefer Optionals over null values, but I understand is a matter of personal preference.

finally, I attach a test code using h2 in memory database an the latest stable version of opencsv (5.5.2) as a proof of my comments about the boolean and bigint:

You can execute it and see that both null values are rendered as empty strings:

"ID","BOOL_VALUE","BIGINT_VALUE"
"1","true","10000000000"
"2","",""

[Andrew Rucker Jones]

Let me repeat that back to you in one sentence to see if I got it right: you're telling me the mock we had was not accurately mocking the true behavior of a ResultSet. Is that correct?

[Ivan Lorca]

Yes, that is.

[Andrew Rucker Jones]

Then what we really ought to be doing is replacing the mock with an h2 database as you did in your example. Then we can take a look at the tests again.

[Ivan Lorca]

It seems to me that it has gone too far away from the original intend and scope of this collaboration to this project.
IMHO, I don't think is a good idea to use a concrete implementation of a database in unit tests, I think it's better to mock the expected behaviour, as it is now, beside this edge case I detected.
Also, the change you are proposing is a huge refactor that I don't have the time or the will to achive.
So, I deeply thought about it all and I decided to change my strategy: I have rewitten my tests to work with the ResultSet mock both as it is and with my original fix for it. I will make a new Merge Request with just this change, and a different one for fixing the mock, and close this one.
Please fell free to merge any of them if you think is ok.
Thanks for your time.