-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling Arrays #33
Comments
Thanks for that. Where is that We could add Array support just as we have added Blob, Clob support, doesn't look too tricky, and allow arrays or java.util.Collection instances to be mapped to them as parameters. The plugin idea sounds good, happy to look at PR but either way I think Array support should be added to the existing design now. Do you think two PRs? First Array support then plugin support? Are you interested in contributing both? |
I believe the the :: notation is postgres specific. This is one of the reasons I tried to be very limited in the change, only skipping the next value in the string if it is ":". I would understand if you didn't want that specific change though since it is postgres specific. I can try to do some preprocessing on the script before sending it off to rxjava-jdbc. The tricky part with respect to supporting arrays and collections it types. This is one of the reasons I thought supporting it in a plugin might be a quicker and easier means to start with. The problem is types. For arrays, this is a little simpler as we can do type tests on the object:
I am not sure if this is cross database, and things like ints and numbers can likely map to many different types. As for collections though we would need some way for the user to pass in a type for the collection as that information won't otherwise be available at runtime. I guess we could introspect the collection but that could lead to other issues if it is not homogenous... I think handling it with two different PRs makes sense if we can figure out a general sensible way to handle arrays and collections |
I'm happy to add the sql parse change. In terms of a plugin I'm wondering if specifying an action to the builder that if present has to configure everything beyond the sql itself. Something like this: String[] values = new String[] {"hello", "there"};
db.update("INSERT into my_table (key1, array_key) VALUES (?, ?::varchar[])")
.prepare(ps -> {
ps.setString(1, "mykey");
Array arr = ps.getConnection().createArrayOf("VARCHAR", (String[]) values);
ps.setArray(2, arr);
})
... Is that too limiting? |
I think that could work. One could argue that keeping it simple will allow for the most flexibility for implementors. If some utility methods were added for instance:
I think it could provide the best of both worlds. The default behavior covers 90% of the use-cases but if you do have some special handling you can provide it there and then. There is still an argument still to be made for the plugin case, in addition. This would allow someone to add support for Java 8 time types without needing to modify every query. |
Righto then how about this as a plan:
Would you be interested in doing the first PR? I'll do the second PR and we can discuss ideas for the third once those are in. |
Sounds good. I will tackle the first PR. I can likely have it by this weekend or early next week. |
That's great, thanks a lot! |
I've run into a few issues when attempting to handle arrays. The first issue is that I am using jooq to generate the sql for the request, it provides a request like so:
The casting gets detected by NamedParameters has being a parameter with a name. This one is fairly simple to resolve by adding an explicit check for a second ':' like so:
The more difficult issue with handling arrays is that the setParameters currently doesn't handle it. My thoughts on this were that a plugin system, similar to the one available in RxJava, could be provided to give users access to the Prepared Statement and object for unknown types.
This way, for types that can't be converted by setObject, like arrays or Java 8 time types, they could be handled. Also, I am not positive on the cross database implications of handling array types.
What are your thoughts? I can provide a PR if you agree with the direction.
The text was updated successfully, but these errors were encountered: