Skip to content
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

Open
alexwen opened this issue Jul 27, 2015 · 7 comments
Open

Handling Arrays #33

alexwen opened this issue Jul 27, 2015 · 7 comments

Comments

@alexwen
Copy link

alexwen commented Jul 27, 2015

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:

INSERT into my_table (key1, array_key) VALUES (?, ?::varchar[])

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:

                if (c == '\'') {
                    inSingleQuote = true;
                } else if (c == '"') {
                    inDoubleQuote = true;
                } else if (c == ':' && i + 1 < length) {
                    final char iPlusOne = namedSql.charAt(i + 1);
                    if (Character.isJavaIdentifierStart(iPlusOne)) {
                        int j = i + 2;
                        while (j < length && Character.isJavaIdentifierPart(namedSql.charAt(j))) {
                            j++;
                        }
                        String name = namedSql.substring(i + 1, j);
                        c = '?'; // replace the parameter with a question mark
                        i += name.length(); // skip past the end if the parameter
                        names.add(name);
                    } else if (iPlusOne == ':') {
                        i++;
                    }
                }

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.

@davidmoten
Copy link
Owner

Thanks for that. Where is that ?::varchar[] usage documented? Is it standard jdbc?

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?

@alexwen
Copy link
Author

alexwen commented Jul 28, 2015

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:

else if (String[].class.isAssignableFrom(cls)) {
  Array arr = ps.getConnection().createArrayOf("VARCHAR", (String[]) o);
  ps.setArray(i, arr);
}

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

@davidmoten
Copy link
Owner

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?

@alexwen
Copy link
Author

alexwen commented Jul 28, 2015

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:

setObject(rs, index, value)

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.

@davidmoten
Copy link
Owner

Righto then how about this as a plan:

  • a PR with the parsing of :: and setting of array parameters as you specified (lets worry about collection support later, just support arrays). Is the number of array types a problem?
  • a PR with prepare method so people can completely customize PreparedStatement if they need to
  • a PR for pluggable behaviour (to be discussed)

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.

@alexwen
Copy link
Author

alexwen commented Jul 29, 2015

Sounds good. I will tackle the first PR. I can likely have it by this weekend or early next week.

@davidmoten
Copy link
Owner

That's great, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants