Support arrays in BindIn, besides Collection #473 #474
Conversation
| if (arg instanceof Collection) { | ||
| return (Collection<?>) arg; | ||
| } | ||
| if (arg instanceof Object[]) { |
There was a problem hiding this comment.
I'd probably like to see this the other way around: convert collections to an array (using Collection.toArray()) and bind from the array. You can use java.lang.reflect.Array.getLength(array) and Array.get(array, index) and then you don't have to have all this code testing for different array types.
Also, there's a checkstyle error failing the build, because your code editor is using tabs instead of spaces for indentation. This project uses spaces.
|
@qualidafial sorry, I only meant the pull request as a rough draft that needs to be fine-tuned before fully merging, not as a ready-to-go submission... There's a million aspects to how a piece of code is written, like logging, exception catching and throwing, message style, considerations like which situations to check first, or like you said indeed maybe to use an array instead of a list entirely, etc. I can't possibly know how you guys like the code to be at every level, just wanted to write this as a quick POC |
|
Isn't using reflection kinda bad btw? It's slow and hacky, while the type check may be verbose but it's fast and clean |
|
Java reflection hasn't been "slow" for years. Certainly it's slower than static code, but not by the crippling factor that it used to be. Moreover as seldom as If you can demonstrate that use of reflection here adversely affects performance in real-world scenarios, I'd be happy to revisit. But absent any evidence of performance issues, I'd rather use code that is maintainable and easy to understand. |
|
Fair enough, I'll patch it up some more. I'm not saying it'll be perfect though, someone who's more in on things in jdbi's specific domain should finetune it thoroughly anyway |
|
Just a heads up: unless a library is using compile-time annotation processors (e.g. Dagger DI), most libraries that make use of annotations are doing so through reflection. Nearly all annotation-driven APIs I'm aware of do their magic through reflection--JDBI included. |
|
That's a little different though. Accessing annotations via reflection is once per statement, whereas mapping like this will be once per value converted. So it will be many orders of magnitude more common. I still lean towards the readable version first, then we can iterate and improve it as we find bottlenecks, but it's quite reasonable to consider the performance cost. And thanks @TheRealMarnes for giving this a go, don't worry at all if you are iteratively improving it -- that's what code review is for :) |
| return Arrays.asList((boolean[]) arg); | ||
| } | ||
| if (arg instanceof byte[]) { | ||
| return Arrays.asList((byte[]) arg); |
There was a problem hiding this comment.
Are you sure this construct actually works? I believe this is actually creating a List<byte[]> which is not what you want.
|
Needs some test cases. I suspect that adding tests will reveal that the |
|
I'm going to put the casting/reflection utility method in a utility class so it can be cleanly tested. Where do you guys want it to go (package and/or class)? |
|
I think there's a |
|
Dunno, that package seems to be (and the package.html even says so) meant for utilities for the end users. This utility is something that should stay out of the end user's way. Maybe I should make a util.internal package? |
|
Or just a different class in the same package -- that way you can make it package-private and not visible to end users. |
|
Not the cleanest from a project files pov, but that does make it completely invisible to users. I like that idea more :P |
|
For some reason I can't get the code to compile locally, in intellij or maven, because of missing dependencies (org.skife.jdbi.rewriter package among others). I'll just push and see if jenkins starts building it or anything. I'm not very familiar with github or this project |
|
|
||
| if (obj.getClass().isArray()) { | ||
| final int length = Array.getLength(obj); | ||
| final Object[] out = new Object[length]; |
There was a problem hiding this comment.
You only need to copy the array if obj.getClass().getComponentType().isPrimitive(). Otherwise you can just cast to Object[]
|
Thanks for the catch, I'd forgotten to re-add that |
|
Come to think of it, why not support Iterable<?>? |
|
Supporting Your build errors due to missing Eclipse knows how to figure this out (with a plugin) but unfortunately I don't know how to get IntelliJ set up right. |
|
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-toolchains-plugin:1.1:toolchain (default) on project jdbi: Cannot find matching toolchain definitions for the following toolchain types: Yay... but wait, maven says java 6, the travis file says jdk7? Maybe the jdk version numbering is a little wonky and it's not like I've looked it up, but shouldn't it be the same? |
|
Ah, yeah this needs to be documented better. You have to configure the Maven Toolchains on your dev box: Travis only has openjdk7 type, but it actually has Java 6 still -- and jdbi v2 is based on Java 6, so we use Toolchains to make sure you use the right one: |
|
Yes, that is a rather confusing message. You need to set up Maven
|
|
Also worth noting -- if you don't have Java 6 installed, you don't actually have to go through the trouble of installing it. You can point your jdk6 toolchain at a Java 8 just fine. The downside is if you accidentally use any new features, you won't notice on a local build - but Travis will catch any errors you make. |
|
Yeah, pointing it at my jdk8 is what I'm gonna go with... |
|
Ok, now I run into runtime type exceptions when I run a full test of @bindin (with varargs for starters)... That'll be for another time :/ |
|
Please leave a note of any combinations you find that don't work! We may not fix them all up for jdbi v2, but we are reworking a lot of this for v3, so we'd appreciate failing tests as issues or even a PR for the v3 branch 😄 Feel free to focus on getting just your use case working, and we can sort the rest out later. |
|
For some reason I can't get a screenshot attached here... Anyway, I've pushed the test class for BindIn, it passes on Iterable but fails on varargs and Iterator. The error is java.lang.ClassCastException: [I cannot be cast to java.lang.Iterable |
|
Something's broken in Github's UI so I can't leave a comment on the specific line, but in BindIn.java:
|
|
Something's broken in Github's UI so I can't leave a comment on the specific line, but in BindIn.java:
|
|
Isn't a cast just a manual override of the reference type that doesn't actually involve any real work (like a true conversion would)? I.e. isn't it harmless and wouldn't some utility method, possibly provided by the jdk, need to do the same thing anyway? |
|
Since |
|
Ah, right, I hadn't noticed a mock system was available, I'll take a look tonight, thanks Edit: I forgot to say it actually seems like the code itself isn't doing what I had hoped. testSomethingByIterableHandleVoidWithEmptyList and testSomethingByIterableHandleVoidWithNull fail because the query returns 0 results when it should return 3 (I pass an empty argument -> should become null keyword -> should return the 3 rows where name = null) Strangely it doesn't select the row with name = 'null' either. I'm guessing the argument actually becomes an empty string. |
|
The binding methods on SQLStatement are final... it's making mocking pretty difficult, but I might get there still. |
|
If you need to make them not final, that's an OK change to make. |
|
Alright, thanks, it was starting to look like that would be the only option... strict class design has its drawbacks :P |
|
Well, I'm not quite happy with the EmptyHandling.NULL tests, but I don't think it can get any better. They verify that null is given as argument value to the query, and that's the closest I could get. After that, the most useful thing I could find was the parameterized query with ? and its bound arguments (the null value). I think after that point we can really only cross our fingers and presume the jdbc driver (or whatever) translates a null parameter to the null sql keyword internally, I don't think you can actually get the resulting sql string. Edit: oh right... the failing tests prove that that's not the case... |
|
Don't worry about 100% unit test coverage. As long as there's some sort of functional test making sure the feature behaves as specified that's likely good enough. |
|
Well yeah, but in this case we have a test that clearly shows a feature not working as intended. I'm honestly not sure what to do with that now. The tests show null is definitely bound to the query and debug view shows jdbc will execute a query with ? bound to null, yet the query doesn't seem to turn out as I expect. Could it be h2 doesn't like "x in (null)" too much? |
|
Oh, I misunderstood the problem. It's entirely possible the behavior of that construct varies between h2 and pg. I can take a bit of a look later tonight possibly if you haven't figure it out by then. |
|
Well, I did just boot up one of my own projects that runs an h2 db with mock data, and this happened: So it seems it's a problem with h2. Maybe it's just a thing that isn't just required for postgres, but also only works on postgres. |
|
I think that looks correct? |
|
Then I guess the tests actually mean it's working up to specs?... I suppose what it really needs is an integration test with postgres instead of h2. |
|
Shall I just fix and break down the null handling test in light of this lesson, push it as such, and consider BindIn finally done then? You guys can figure out a way to test the null handling some other time then, and in the meantime mark EmptyHandling.NULL as experimental/untested for the postgres peeps... |
|
What exactly doesn't work right? With |
|
|
||
| final List<Something> out = s.get(new ArrayList<Object>()); | ||
|
|
||
| Assert.assertEquals(4, out.size()); |
There was a problem hiding this comment.
I expect out to be empty here, yes?
|
Well I guess up to there it's working indeed. The thing I'd still like to verify is that it in fact does output the null keyword in the query so that it ends up being "x in (null)", but like I said earlier I don't think we can test for that... Yeah sorry, my thoughts are a mess right now anyway. I guess everything is in order? |
| { | ||
| final SomethingByIterableHandleNull s = handle.attach(SomethingByIterableHandleNull.class); | ||
|
|
||
| final List<Something> out = s.get(null); |
There was a problem hiding this comment.
This might be considered a programming error -- should we really assume null is empty list instead of a coding error?
|
Looking at the failing test, I think the test is wrong. An empty input should match no rows no matter what data is in the table. |
Yes, I'm changing that test atm. I mean the functionality itself is apparently working as intended. |
You could I feel this is getting out of hand though. I'm good to just merge this. |
|
Yes, I think the existing test (once fixed) is sufficient. No need for |
|
agreed |
|
Merged, thanks! |
|
Just for my information, around when are you planning to release 2.77? I kinda need the bindin fix in a project I'm working on (hence how I found the bug and why I did any of this to begin with) |
|
@TheRealMarnes sorry was on vacation and lost track of it, just did the release :) |
|
Awesome, thanks |
#473