Support for executing stored procedures and functions in Derby #524

MMatten · 2016-12-03T13:49:00Z

Support stored and function parameters in Derby.

Initial draft of one approach to using only JDBC method for querying Derby object metadata.

Potential for reuse for a generic DB adapter(?)

javornikolov

The overall approach looks good to me. I added some technical remarks.

javornikolov · 2016-12-03T15:03:31Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

-        return readIntoParams(tableOrViewName.toUpperCase(), qry);
+    public Map<String, DbParameterAccessor> getAllColumns(String tableName)
+                                                        throws SQLException {
+        DatabaseTable t = new DatabaseTable(getConnection(), getConnection().getSchema(), tableName.toUpperCase());


That temp variable is only used at next line so possibly we can skip it and call .getParams() here directly.

Yes. Good point.

javornikolov · 2016-12-03T15:17:22Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        public boolean exists() throws SQLException {
+            ResultSet rs = con.getMetaData().getFunctions(null, name.getSchemaName(), name.getObjectName());


The cursor is not being closed here. Perhaps the helper method from the other PR can be reused here.

javornikolov · 2016-12-03T15:18:14Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        protected Direction getParamDirection(ResultSet rs)
+                                        throws SQLException {


I think the method name and throws clause is short enough to fit in single line here.

javornikolov · 2016-12-03T15:20:23Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        public boolean exists() throws SQLException {
+            ResultSet rs = con.getMetaData().getProcedures(null, name.getSchemaName(), name.getObjectName());


ResultSet remains unclosed.

javornikolov · 2016-12-03T15:24:16Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        } else {
+            DatabaseFunction f = new DatabaseFunction(getConnection(), getConnection().getSchema(), name);
+            if (f.exists()) {
+                o = f;


Here we go again in multiple levels of nesting and some single-letter variable names. The if/else if/else in the other PR looked a bit easier to read.

It's not exactly the same case as I have more than one statement at some of the branching points.

I'll see how I can squash it a bit.

javornikolov · 2016-12-03T15:26:00Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            switch (rs.getInt("COLUMN_TYPE")) {
+                case DatabaseMetaData.procedureColumnIn:
+                    dir = Direction.INPUT;
+                    break;


What about shortcut return to avoid the multiple break statements and the temp variable?

I will need a default option with a return in the switch to ensure compilation. I.e. make the switch statement be: -

switch (rs.getInt("COLUMN_TYPE")) { case DatabaseMetaData.procedureColumnIn: return Direction.INPUT; case DatabaseMetaData.procedureColumnInOut: return Direction.INPUT_OUTPUT; default: return Direction.OUTPUT; }

It can be either return in the default option or it can be return statement after the switch (both compile fine).

But good that you turned attention into that: right now it returns null in case of unknown type. Shouldn't it be throwing an exception?

Shouldn't it be throwing an exception?

Yes. Theoretically it can't happen but I'll raise a SQLException for the default option.

javornikolov · 2016-12-03T15:35:18Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

-        try (PreparedStatement dc = getConnection().prepareStatement(query)) {
-            dc.setString(1, tableOrViewName);
+    private abstract class DatabaseObject {
+        protected Connection con;


Hmm, we have many names and abbreviations for the connection (con, conn, ...).

Technically here we have a direct access here to the parent getConnection() of DerbyEnvironment (since it's a non-static inner class of it).

I can get at that method with DerbyEnvironment.this.getConnection() but then end up with long expressions.
E.g.
DerbyEnvironment.this.getConnection().getMetaData().getFunctionColumns(null, name.getSchemaName(), name.getObjectName(), "%");

What preferred practice here? Introduce another variable to reference the result of DerbyEnvironment.this.getConnection() or just break up the expression over multiple lines. E.g.

ResultSet rs = DerbyEnvironment.this.getConnection().getMetaData() .getFunctionColumns(null, name.getSchemaName(), name.getObjectName(), "%");

You should be able to directly call getConnection() instead of DerbyEnvironment.this.getConnection().

Other ideas to make lines shorter:

Do we ever need the connection directly or we always call getMetaData on it?

You may have a protected method getMetaData() -> getConnection().getMetaData().

Instead of ResultSet rs = you may have a method getObjectMetaData() -> return getMetaData().....

javornikolov · 2016-12-03T15:40:53Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        private DatabaseObject(Connection conn, String defaultSchemaName, String objName) {
+            con = conn;
+            name = new DbObjectName(defaultSchemaName, objName);


That DbObjectName can come directly from the constructor.

javornikolov · 2016-12-03T15:42:06Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        abstract protected ResultSet getColumns() throws SQLException;
+
+        public Map<String, DbParameterAccessor> getParams() throws SQLException {
+            ResultSet rs = getColumns();


This rs is never closed.

Ok. Thanks.

MMatten · 2016-12-03T20:26:13Z

I've made an attempt at addressing all of these points and have force pushed the revisions.

MMatten · 2016-12-03T20:28:57Z

Ah. I didn't see you latest comments until I'd pushed revisions. I'll go through those now.
...and realised I've left some debugging code in there.

MMatten · 2016-12-03T21:51:04Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        }
+    }
+
+    private class DbObjectName {


May be this should be DatabaseObjectName to align with the other new inner classes...?

Yes, sounds like good idea.

Update applied.

MMatten · 2016-12-03T21:53:52Z

I've applied your ideas. Let me know what you think.

javornikolov · 2016-12-03T22:06:18Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        public ResultSet getColumns() throws SQLException {
+            ResultSet rs = getMetaData().getProcedureColumns(null, name.getSchemaName(), name.getObjectName(), "%");


Do we need the rs variable?

Oops. Thanks.

javornikolov · 2016-12-03T22:10:15Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        public boolean exists() throws SQLException {
+            try (ResultSet rs = getObjects()) {


Duplicated code (it's the same in all child classes).

Yes. I can pull it back into the abstract base class.

javornikolov · 2016-12-03T22:14:13Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            return getConnection().getMetaData();
+        }
+
+        abstract protected ResultSet getObjects() throws SQLException;


The name is a bit confusing: since in general we intend to represent a single database object.

Agreed. I didn't really like it that much. I'm now thinking of having getObjectMetaData() and getDBMetaData().

In fact getDBMetaData(), from the connection, and getMetaData() for the object.

javornikolov · 2016-12-03T22:23:37Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+                    return Direction.OUTPUT;
+                default:
+                    throw new SQLException("Unpexpected procedure parameter type for parameter " +
+                                  rs.getString("COLUMN_NAME"));


What indentation conventions are being used here?

Again, nothing very scientific here. I can change it to align with the opening parenthesis. I.e.:

default: throw new SQLException("Unpexpected procedure parameter type for parameter " + rs.getString("COLUMN_NAME"));

javornikolov · 2016-12-03T22:24:56Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+    }
+
+    public Map<String, DbParameterAccessor> getAllProcedureParameters(String callableName)
+                                                                       throws SQLException {


What is the rule of indentation here?

It's a bit random. The approach was merely to align the opening brace { with where it would be if the method had no throws clause.

What approach do you take?

My approach for throws formatting is also not very streamlined. Usually I try to keep all indentations at position divisible by 4 spaces (or whatever the unit is). Things I might do for that case:

public Map<String, DbParameterAccessor> getAllProcedureParameters(String callableName) throws SQLException { // 2 indentation (just a single 1 would align with the method code and won't stand out well)

or

public Map<String, DbParameterAccessor> getAllProcedureParameters( String callableName) throws SQLException { // with multiple params public Map<String, DbParameterAccessor> getAllProcedureParameters( String callableName, String param2, String param3) throws SQLException, SQLException2 { // code }

At some point I used to do something like following but I think now I would rather take the approach in the previous sample.

public Map<String, DbParameterAccessor> getAllProcedureParameters(String callableName) throws SQLException {

javornikolov · 2016-12-03T22:53:46Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        if (dbproc.exists()) {
+            return dbproc.getParams();
+        } else {
+            DatabaseFunction dbfunc = new DatabaseFunction(objName);


Hmm, that still looks a bit complicated if compared to the other methods here. And indeed with this design of exists it's not quite the same as it used to be in previous PR. What about having one more helper method to get the object. E.g. something like:

DatabaseObject findStoredRoutine(objName) { List<DatabaseObject> prioritisedCandidates = Arrays.asList( new DatabaseProcedure(objName), new DatabaseFunction(objName)); for (DatabaseObject object : prioritisedCandidaes) if (object.exists()) { return object; } } throw new SQLException("Procedure/function " + objName + " does not exist"); }

That's nice. Thanks. I'll copy it in.

Would you usually include the blank lines in this method?

Yes, I would usually include the blank lines. Maybe the 1st one could be omitted since the above statement is multi-line with some indentation with 2nd, 3rd lines. But if the List... was all on single line - the for loop block wouldn't stand out very well without an empty line in between.

MMatten · 2016-12-04T11:10:59Z

More polishing applied.

javornikolov · 2016-12-04T11:07:23Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+    public Map<String, DbParameterAccessor> getAllColumns(String tableName)
+            throws SQLException {
+        return new DatabaseTable(new DatabaseObjectName(getConnection().getSchema(),
+                                 tableName.toUpperCase())).getParams();


Here the the two parameters of DatabaseObjectName are not quite aligned. I would approach that maybe like that:

return new DatabaseTable(new DatabaseObjectName( getConnection().getSchema(), tableName.toUpperCase())).getParams(); // or return new DatabaseTable(new DatabaseObjectName( getConnection().getSchema(), tableName.toUpperCase())).getParams();

Ok. It's subjective and tricky stuff. Do you know of anything in checkstyle that we can use to pick a standard?

I'll choose one of your options for now.

I'm not sure if checkstyle has something good enough.

There are some related notes in the style guide: http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#262

javornikolov · 2016-12-04T11:18:40Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            name = objName;
+        }
+
+        protected DatabaseMetaData getDBMetaData() throws SQLException {


Should it be DB or Db? I think we have of both around, maybe Db is more common (like DbParameterAccessor).

Ok. Will update.

javornikolov · 2016-12-04T11:23:45Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        @Override
+        protected Direction getParamDirection(ResultSet rs) throws SQLException {
+            int paramType = rs.getInt("COLUMN_TYPE");


In DatabaseProcedure we don't have that local variable.

javornikolov · 2016-12-04T11:29:00Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+                    return Direction.RETURN_VALUE;
+                default:
+                    throw new SQLException("Unpexpected function parameter type for parameter " +
+                                  rs.getString("COLUMN_NAME"));


That's aligned a bit differently in DatabaseProcedure.

I'm wondering about how to avoid the small bits of duplication here - the throw exception is the same, the get.. column type is the same. Also there is a similar pattern (switch statement).

I think that there are some options to tackle that too. What do you think?

As parameter direction is not really appropriate for tables I was originally thinking of having another level of abstraction, possible DatabaseExecutable or DatabaseCallable and pushing as much as I could into a method in that class. I still can't quite put my finger on how to have a single place to throw the exception if the param type from the get...Columns is invalid. Unless we were to return null for a Direction value from the method that contains the specific switch.

Could be some map (int -> direction) like we have with type mapping in db environments.

I'm thinking of using something like guava's ImmutableMap.of() or ImmutableMap.builder to initialise a static Map from a list of literals. Otherwise with HashMap it has to be done in a method (perhaps the DatabaseProcedure/Function constructor).

Then we have a method in the new DatabaseExecutable (or DatabaseRoutine?) that gets the Direction value from the JDBC parameter type int. If it's not in the map then throw the exception.

It still sounds like I have something like

if (getParamDirection(jdbcIntValue) == null) { throw new SQLException("Invalid parameter type"); } return (getParamDirection(jdbcIntValue);

to initialise a static Map from a list of literals

Why should it be a static one? I'm not sure the performance overhead would be so terrible even with non-static one.

Otherwise with HashMap it has to be done in a method

That can even be in the getXXX method.

It still sounds like I have something like

if (getParamDirection(jdbcIntValue) == null) { throw new SQLException("Invalid parameter type"); } return (getParamDirection(jdbcIntValue);

I think yes. Maybe with a variable to avoid calling the getXXx twice.

Maybe member variable (static or not) would makes some sense. Just I don't see that yet, good to start with a more obvious implementation.

BTW, initialization logic of static members can be done in static initialiser like:

static { map.put(key, value); ... }

Or to do an assignment by call to static method.

But in general static members are not supported in non-static inner classes. If desired in our case - this could rather be done in the parent class.

Initializer blocks may be actually used in non-static case too:

abstract class Base { protected final Map<String, String> map = new HashMap<>(); } class Child1 extends Base { { map.put("0", "value1"); map.put("1", "value2"); map.put("2", "value3"); } }

Actually, after all - going through a Map isn't needed here (it's just an option). Using switch is perfectly fine and perhaps superior in terms of performance: could be 2nd method (int) -> Direction /returning null for unknown type/. Then the (ResultSet) -> Direction could be generalized and captured in a single place only (the base class).

javornikolov · 2016-12-04T11:37:32Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+    public Map<String, DbParameterAccessor> getAllProcedureParameters(String callableName)
+            throws SQLException {
+        String name = callableName.toUpperCase();
+        return findStoredRoutine(new DatabaseObjectName(getConnection().getSchema(), name)).getParams();


That new DatabaseObjectName(getConnection().getSchema(), somename.toUpperCase()) is a bit long and we have in one more place. So could we fine tune that too :-)

Yes. I can't think of a nice short name. How's this one: -

DatabaseObjectName createDbObjectNameWithCurrentDefaultSchema(String objName) throws SQLException { return new DatabaseObjectName(getConnection().getSchema(), objName); }

Maybe it could be just parseDatabaseObjectName or even parseObjectName for now. Can be renamed some day if some day we have more options and risk of mis-interpretation.

Possibly objeName.toUpperCase() can be included here (to be honest I'm not sure about case sensitivity in Derby but if uppercase of name is needed, I guess it would be fine for schema name too).

Object names need to be upper case when using the JDBC get... methods. I guess Derby translate names to upper case when objects are created.

MMatten · 2016-12-10T00:46:44Z

I draft another approach to get rid of the dummy method returning Direction.INPUT for DatabaseTable types. Any thoughts on this approach?

javornikolov

Nice, it looks quite good now!

A few minor remarks commented in-line.

javornikolov · 2016-12-10T09:21:29Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            super(objName);
+        }
+
+        abstract protected Direction columnTypeToDir(int columnType);


Some wondering whether Dir is better than Direction here?

You're right. I'll expand that.

javornikolov · 2016-12-10T09:32:54Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+    }
+
+    DatabaseObjectName createDbObjectNameWithCurrentDefaultSchema(String objName) throws SQLException {
+        return new DatabaseObjectName(getConnection().getSchema(), objName);


When we call that we allays do .toUpperCase(). What about moving that here?

What about moving the parsing logic from DatabaseObjectName c-tor to here?

Ah, didn't address these yet...

What about moving the parsing logic from DatabaseObjectName c-tor to here?

What would that do for us?

What about adding a parse method to the DatabaseObjectName class although it can't be static?

What about moving the parsing logic from DatabaseObjectName c-tor to here?

What would that do for us?

Would allow more expressive name for the DatabaseObjectName construction method.

There is a general rant about factory methods vs constructors, and avoiding complex logic in constructors. As we already have a factory method (create/build ... with current default schema): it looks logical to keep non-core construction details outside the c-tor.

It's interesting to have a look at Kotlin Data Classes. Unfortunately not available in Java language itself (maybe some day).

What about adding a parse method to the DatabaseObjectName class although it can't be static?

The issue is we can't use non-static method before construction... But it's possible to make DatabaseObjectName class static. Or the parsing method can be outside the class (as we now have buildDatabaseObjectName).

The issue is we can't use non-static method before construction... But it's possible to make DatabaseObjectName class static.

We have no reliance upon a parent class object instance at the moment so that's also possible.

Do you mean that data-like classes should just really have a constructor that assigns values to internal fields and getters to retrieve them?

Yes, if this is simpler, trivial and obvious representation which does the job.

Other points might be:

In sense of simplicity - avoid mixing multiple responsibilities.

Consistency with other common API (e.g. QName, Integer, LocalDateTime - even though these have other additional stronger reasons to that approach)

Indeed, it's fairly subjective matter and the parsing is simple. It's hard to make a strong case (e.g. like in http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/).

What matters now is the current context so if things look better as they are - no problem to be that way.

It makes sense.

My only doubt is that having this common parsing outside of the class seems to inhibit potential reuse of the DatabaseObjectName class. If other classes were to instantiate it they'd need the same parsing method (duplicated) or I guess moved into a factory class or some other initialiser method. What would you tend to do in this case?

Yes, some kind of factory/builder/parser class or method comes to my mind too. Parsing methods or c-tors inside the same class itself can't solve such problem. Or it could be a static method with a different name.

It's something to look into when we extend the usage to another place.

Thanks. I've pushed some updates for the above discussion.

javornikolov · 2016-12-10T09:59:37Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        protected DbParameterAccessor createColumnAccessor(String paramName, ResultSet rs)
+                throws SQLException {
+            return createDbParamAccessor(paramName, rs, getParamDirection(rs));


Perhaps we can push that implementation up to DatabaseObject (by adding getParamDirection -> INPUT to DatabaseTable).

We did have that earlier. It still didn't feel right having this method that didn't use any of it's inputs/params. What do you think about dummy methods like this?

I think it's OK in that case.

Do you mean you think it's OK to have a dummy one? Or it OK to remove it (as it is right now)?

The notion of Direction seems contrived and a bit ugly for tables. I can only think DbFit was originally developed with stored procs in mind and tables/views/queries were added later.

In my understanding all accessors have a Direction (all JDBC statements are like that) - these are accessors to parameters of SQL statements. Unless we split them into separate Input and Output accessors which is also possible but a separate topic.

Well, getColumns does not return a COLUMN_TYPE column so table columns don't have direction according to JDBC.

I meant direction in terms of PreparedStatement parameters. I agree that columns and stored proc parameters have different properties, I'm trying to make the point that none of them are properties of database statements at all.

Right, yes. I see what you mean. So perhaps our references to parameters, really being statement parameters, is a little confusing.

I meant direction in terms of PreparedStatement parameters.

We're using DbParameterAccessor for non PreparedStatement/CallableStatement too. E.g. for query results and we use a dummy INPUT Direction for these may be we could clean this up some time.

javornikolov · 2016-12-10T10:24:00Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        abstract protected Direction columnTypeToDir(int columnType);
+
+        protected Direction getParamDirection(ResultSet rs) throws SQLException {


An alternative idea about that mapping: Direction getParamDirection(ResultSet rs) can be abstracted in separate interface and in child classes we can just plug different implementations of it (e.g. via initialization of common member via the constructor or in some other way).

Maybe that doesn't make sense in our particular case here. I'm just sharing a thought which came to me while wondering about alternatives without a deep 3-level hierarchy (substituting inheritance with composition in this case).

javornikolov · 2016-12-10T10:30:14Z

The compilation error should be possible to handle via ? extends T (a bit subtle caveat of Java generics).

javornikolov · 2016-12-10T10:58:04Z

The compilation error should be possible to handle via ? extends T (a bit subtle caveat of Java generics).

Or change the type in the list.
Interesting enough: looks like we're not using exists for DatabaseTable... Maybe it's fine that way, I'm not sure what happens if we reach to getting table columns of non-existent table?

MMatten · 2016-12-10T11:42:18Z

The compilation error should be possible to handle via ? extends T (a bit subtle caveat of Java generics).

Or change the type in the list.

It only fails to compile with JDK 7, but with 8 it's fine.

I've update the List to be based upon DatabaseExecutable.

MMatten · 2016-12-10T12:04:12Z

Interesting enough: looks like we're not using exists for DatabaseTable... Maybe it's fine that way, I'm not sure what happens if we reach to getting table columns of non-existent table?

I've not yet changed the behaviour of getting table column accessors. Currently it returns an empty Map and the behaviour is consistent with how the other adaptors work for tables and stored code.

We can instead make it call the exists() for DatabaseTables and throw an exception instead. What do you think?

javornikolov · 2016-12-10T12:08:19Z

We can instead make it call the exists() for DatabaseTables and throw an exception instead. What do you think?

Let's keep that behavior as is. If we change it we should rather change all other adapters to keep them consistent.

Since we're not using DatabaseTable.exists() - I guess it makes sense to drop that code and make it relevant for stored routines only (for now).

MMatten · 2016-12-10T12:28:03Z

Since we're not using DatabaseTable.exists() - I guess it makes sense to drop that code and make it relevant for stored routines only (for now).

And revert behaviour for procs/funcs too?

javornikolov · 2016-12-10T12:35:57Z

Since we're not using DatabaseTable.exists() - I guess it makes sense to drop that code and make it relevant for stored routines only (for now).

And revert behaviour for procs/funcs too?

Could be just moving the abstract exists to *Executable.

MMatten · 2016-12-10T13:00:04Z

Tidied up (apart from commits still to squash).

javornikolov · 2016-12-10T14:49:56Z

Could be just moving the abstract exists to *Executable.

Or it could be some default implementation in the base class throwing 'not implemented yet'. And maybe it's possible to get rid of DatabaseExecutable (if nothing specific remains for it). After all - having exists() in DatabaseTable is not so bad (though leading to some confusion where it's used).

MMatten · 2016-12-10T16:17:56Z

And maybe it's possible to get rid of DatabaseExecutable (if nothing specific remains for it). After all - having exists() in DatabaseTable is not so bad (though leading to some confusion where it's used).

I think I'll still have some common functionality at this level. Let me push the latest version to get a view of what it all looks like.

javornikolov · 2016-12-11T22:41:04Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            super(objName);
+        }
+
+        protected DbParameterAccessor createColumnAccessor(String paramName, ResultSet rs)


The implementation looks the same in the parent.

javornikolov · 2016-12-11T22:41:46Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            return getDbMetaData().getColumns(null, name.getSchemaName(), name.getObjectName(), "%");
+        }
+
+        protected Direction columnTypeToDirection(ResultSet rs) {


Missing @Override

javornikolov · 2016-12-11T22:47:03Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            return jdbcTypeToDirection(rs.getInt("COLUMN_TYPE"));
+        }
+
+        abstract protected Direction jdbcTypeToDirection(int jdbcType);


Would it work if we use the same columnTypeToDirection name here? Also I'm not quite sure about the name of jdbcType.

Would it work if we use the same columnTypeToDirection name here?

Yes. Does overloading internal method names make it harder to follow though?

I'm about to push a new version.

javornikolov · 2016-12-12T11:39:14Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        protected Direction columnDirection(ResultSet rs) throws SQLException {
+            Direction columnDir = getColumnDirection(rs);
+            if (columnDir == null) {
+                throw new SQLException("Invalid column/parameter type");


I think we use used to have the column name in the exception message in some of the earlier drafts. But maybe it's OK to keep it that way.

I can pass in the name to this method too.

I can pass in the name to this method too.

If you consider passing the name: introducing getColumnName(rs) is also an option (to avoid carrying around too many parameters).

If you consider passing the name: introducing getColumnName(rs) is also an option (to avoid carrying around too many parameters).

An interesting point with this is that our use of defaultIfNull is only really relevant for function return values. It wouldn't be obvious if we just define it in DatabseObject. May be we should have a default in DatabaseObject and override it in DatabaseFunction. What do you think about obviousness vs conciseness?

Good point indeed. Perhaps I would keep defaultIfNull in DatabaseObject only. We replace nulls in order to avoid complications when working with the map keys. There are other attributes which clearly distinguish the return values.

I mean, the null name specific doesn't seem to be something so important in order to need to be emphasized. It's just kind of type conversion to a not null value.

javornikolov · 2016-12-12T11:49:15Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            return columnTypeToDirection(rs.getInt("COLUMN_TYPE"));
+        }
+
+        abstract protected Direction columnTypeToDirection(int columnType);


Would it work if we use the same columnTypeToDirection name here?

Yes. Does overloading internal method names make it harder to follow though?

If they're doing the same (just on different format of parameters): it seems fine if name is the same.

Looking at the additional complications to avoid the duplication of rs.getInt("COLUMN_TYPE") I'm wondering didn't we go too far for that simple case.

Maybe at the cost of little duplication we could take a step back and move that down to the children: switch(rs.getInt("COLUMN_TYPE")). Or alternatively declare a getColumnTypeId(ResultSet) inside DatabaseObject (despite that we don't use it for tables). Maybe it's a good tradeoff since this way we can keep the class hierarchy 2-levels only and perhaps drop the int->Direction mapping function.

In any case the current version looks quite well.
What do you think?

Would it work if we use the same columnTypeToDirection name here?

Yes. Does overloading internal method names make it harder to follow though?

If they're doing the same (just on different format of parameters): it seems fine if name is the same.

OK. I'll align them.

Maybe at the cost of little duplication we could take a step back and move that down to the children: switch(rs.getInt("COLUMN_TYPE")).

Strangely it seems that the Derby driver actually does include a COLUMN_TYPE column in result sets for table columns, after all. But it's not documented as so should probably not be relied upon and if we're looking for any reuse for a generic adapter we should avoid it too.

Maybe at the cost of little duplication we could take a step back and move that down to the children: switch(rs.getInt("COLUMN_TYPE")). Or alternatively declare a getColumnTypeId(ResultSet) inside DatabaseObject (despite that we don't use it for tables). Maybe it's a good tradeoff since this way we can keep the class hierarchy 2-levels only and perhaps drop the int->Direction mapping function.

I'm not sure about it. I think my own preferred version was actually where we only had any methods relating to Direction in DatabaseExecutable and below.

For auto-generated primary keys some databases use DbAutoGeneratedKeyAccessor. But in e.g. in Oracle it's different - and we have INSERT ... RETURNING INTO statement with both input and output parameters.

We have code which is agnostic to the accessor direction (e.g. in DbObject and others based on it).

In DbParameterAccessor itself: an object either can do ... get() or set(Object) depending on its direction (the unsupported operation throws exception when called). So that's another place which may lead to some unease.

In order be able to treat things in generic way we sometimes end up with dummy behaviors (methods returning null/exception,unused parameters, ...).

Maybe a way to view that is adapter pattern: in order to be able to plug something into a common processor, we add some attachments so that it complies with the common interface.

In the particular case it's relatively small private code. Taking shortcuts to make things shorter and easier to read may make sense. (As you noted, shorter is not necessarily simpler - if it leads to obscurity, it's not so good).

Maybe the new class names do not help to clarify what their purpose is. Maybe they should be more aligned with the types of statements that we're actually executing: -

DML targeting tables (CUD, not R).

Executing stored code.

Would that make names like: -

StatementTargetObject

StoredCodeExecutionStatementTargetObject

DataModificationStatementTargetObject

more valuable even though these are ridiculously long?

The new classes are details supporting implementation of:
getAllProcedureParameters(),
getAllColumns().

The result is metadata about object (table/view) columns or stored procedure parameters. We don't want to couple too much to assumptions about specific uses of these metadata. (It may be DML statement, but it may be read-only metadata inspection, or supporting Query execution).

Yes. It's just the Direction that's been confusing me. For queries and inspections I don't think we use DbParameterAccessor. So for tables it's always DML (not SELECT).

So Direction for tables is for a very specific purpose.

Maybe we should change the types returned as database metadata in future, and convert/adapt them to accessors as separate step when needed.

Yes, for tables currently it' seems DML only. Hence the direction is by default INPUT. Sometimes we have additional manipulations on top of the returned lists (splitting in/out parameters as in+out, cloning for the purpose of auto-generated keys, etc.).

MMatten · 2016-12-13T09:54:11Z

Looking at the additional complications to avoid the duplication of rs.getInt("COLUMN_TYPE") I'm wondering didn't we go too far for that simple case.

Maybe at the cost of little duplication we could take a step back and move that down to the children: switch(rs.getInt("COLUMN_TYPE")).

I'll apply this and push again.

MMatten · 2016-12-13T09:55:32Z

I think we can reasonably merge createDbParamAccessor into createColumnAccessor. What do you think?

javornikolov · 2016-12-13T10:29:35Z

I think we can reasonably merge createDbParamAccessor into createColumnAccessor. What do you think?

Maybe not a bad idea. Possibly also param name can be extracted from within.

javornikolov · 2016-12-13T19:27:05Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

-                allParams.put(NameNormaliser.normaliseName(columnName), dbp);
+            try (ResultSet rs = getColumns()) {
+                while (rs.next()) {
+                    String paramName = getColumnName(rs);


Things look good so far. What do you think about moving the getColumnName(rs) inside createColumnAccessor? That way it will be closer to other similar activities. And here we can take the name from the accessor after creating it.

So like this?

public Map<String, DbParameterAccessor> getParams() throws SQLException { Map<String, DbParameterAccessor> allParams = new HashMap<String, DbParameterAccessor>(); try (ResultSet rs = getColumns()) { while (rs.next()) { DbParameterAccessor accessor = createColumnAccessor(rs); allParams.put(normaliseName(accessor.getName()), accessor); } } return allParams; } protected DbParameterAccessor createColumnAccessor(ResultSet rs) throws SQLException { String paramTypeName = rs.getString("TYPE_NAME"); return createDbParameterAccessor(getColumnName(rs), columnDirection(rs), typeMapper.getJDBCSQLTypeForDBType(paramTypeName), getJavaClass(paramTypeName), rs.getInt("ORDINAL_POSITION") - 1); }

Yes. The allParams.put(...) is maybe short enough to fit on single line.

javornikolov · 2016-12-13T19:33:47Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+            try (ResultSet rs = getColumns()) {
+                while (rs.next()) {
+                    String paramName = getColumnName(rs);
+                    allParams.put(NameNormaliser.normaliseName(paramName),


Possibly we can shrink length of normaliseName calls via static import.

javornikolov · 2016-12-13T20:44:13Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        abstract protected ResultSet getColumns() throws SQLException;
+
+        public Map<String, DbParameterAccessor> getParams() throws SQLException {
            Map<String, DbParameterAccessor> allParams = new HashMap<String, DbParameterAccessor>();


This should be possible to construct by new HashMap<>() (types will be resolved automatically).

javornikolov · 2016-12-13T22:48:29Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+        }
+
+        @Override
+        abstract protected Direction getColumnDirection(ResultSet rs) throws SQLException;


Abstract stuff doesn't need to be overridden (it's inherited). But here DatabaseExecutable is already completely hollow so it can be dropped.

Right, yes. I'd still like to see if I can have method names that more closely match the intent for the columns (PreparedStatement / CallableStatement parameters) to give some more content to the idea of Direction.

I guess maybe Direction itself should be StatementParameterDirection or the DBEnvironment API method names should be more representative. That's a different PR.

Maybe there's some stuff local to this PR. I'll have a play around...

javornikolov · 2016-12-14T20:30:11Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java

+
+        protected Direction columnDirection(ResultSet rs) throws SQLException {
+            Direction columnDir = getColumnDirection(rs);
+            if (columnDir == null) {


I just found out a java.util.Objects utility function which could be handy here:

return requireNonNull(getColumnDirection(rs), "Our error message....");

That's neat. I guess we just let the NullPointerException just propogate.

Yes, NPE instead of SQLException looks just fine to me.

This method isn't defined as throwing an NPE.

https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull(T,%20java.lang.String)

So I'm not required to throws NullPointerException.

We still have SQLExceptions from getColumnDirection because the the rs field access.

Yes, it's not needed to declare throws for children of RuntimeException. It's fine that we have SQLException for the rest.

javornikolov · 2016-12-14T21:15:51Z

dbfit-java/derby/src/main/java/dbfit/environment/DerbyEnvironment.java


-            ResultSet rs = dc.executeQuery();
+        public Map<String, DbParameterAccessor> getParams() throws SQLException {
            Map<String, DbParameterAccessor> allParams = new HashMap<String, DbParameterAccessor>();


Should be fine to call new HashMap<>() here, no need to list types on the right side (I think we already did that somewhere).

Yes. I've yet to push that one.

MMatten · 2016-12-14T21:59:29Z

I'll leave the names as they are at this stage. Any other thoughts on this PR?

javornikolov · 2016-12-14T22:09:39Z

I'll leave the names as they are at this stage. Any other thoughts on this PR?

It looks good to me 👍 No more comments from my side.

I think we can go on and merge it. What do you think? (If for some reason we conclude that alternative implementation based on plain SQL is better - it won't be a problem to change it).

MMatten · 2016-12-14T22:17:28Z

Regarding the naming I'm now leaning more towards calling stuff parameter rather than column. I.e.: table parameters and procedure/function parameters.

In the API too perhaps methods like getTableParameters and getStoredRoutineParameters would make things clearer. They're available parameters for statements affecting those target objects.

What do you think?

MMatten · 2016-12-14T22:20:11Z

I think this PR makes a good foundation for a generic adapter too. May be we should use the SQL query one for Derby and the JDBC one for Generic (as per the jdbcSlim project). Any thoughts?

javornikolov · 2016-12-14T22:34:57Z

Regarding the naming I'm now leaning more towards calling stuff parameter rather than column. I.e.: table parameters and procedure/function parameters.

In the API too perhaps methods like getTableParameters and getStoredRoutineParameters would make things clearer. They're available parameters for statements affecting those target objects.

What do you think?

Indeed, in some places we already have parameter as name linked to both sp parameters and table columns. As discussed, maybe makes sense to separate the metadata type from the one used to build statement parameters. Even in that case, there is a lot of commonality between table columns and sp parameters: and a common name for both would be needed I guess...

In #463 I can see I've introduced another type DbParameterDescriptor (much leaner than DbParameterAccessor)... That's unfinished concept though, and I don't remember details of what I had in mind by that time.

javornikolov · 2016-12-14T22:38:44Z

I think this PR makes a good foundation for a generic adapter too. May be we should use the SQL query one for Derby and the JDBC one for Generic (as per the jdbcSlim project). Any thoughts?

Yes, it's would be good foundation for a generic adapter (though in general it's not clear yet what else would be able compatible). Unless we end up with extra features with the SQL-query alternative, likely it would make sense to reuse it for Derby (otherwise we end up with extra code to maintain for the same purpose).

javornikolov · 2016-12-16T08:10:12Z

I'm merging that... Thank you @MMatten!

MMatten mentioned this pull request Dec 3, 2016

Support stored procedure and function parameters in Derby #522

Closed

javornikolov requested changes Dec 3, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from a4f31b6 to b4dd444 Compare December 3, 2016 20:25

MMatten force-pushed the support-derby-stored-unit-calls branch from b4dd444 to 84f7244 Compare December 3, 2016 21:49

MMatten commented Dec 3, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from 84f7244 to 3281710 Compare December 3, 2016 22:10

javornikolov requested changes Dec 3, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch 3 times, most recently from 65e26f8 to ef363c8 Compare December 4, 2016 11:10

javornikolov reviewed Dec 4, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from f4d2df9 to 1a1ad42 Compare December 10, 2016 00:44

javornikolov reviewed Dec 10, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from 1a1ad42 to 9840a52 Compare December 10, 2016 10:54

MMatten force-pushed the support-derby-stored-unit-calls branch from 9840a52 to 20c661b Compare December 10, 2016 12:59

MMatten force-pushed the support-derby-stored-unit-calls branch from 9fa6ffc to 79fd1dc Compare December 11, 2016 22:17

javornikolov reviewed Dec 11, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from 79fd1dc to ff51fc6 Compare December 12, 2016 09:34

javornikolov approved these changes Dec 12, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from ff51fc6 to cee9af0 Compare December 13, 2016 10:49

javornikolov reviewed Dec 13, 2016

View reviewed changes

MMatten force-pushed the support-derby-stored-unit-calls branch from cee9af0 to 836a67b Compare December 14, 2016 19:44

javornikolov reviewed Dec 14, 2016

View reviewed changes

Support stored and function parameters in Derby

e98c853

MMatten force-pushed the support-derby-stored-unit-calls branch from 836a67b to e98c853 Compare December 14, 2016 21:22

javornikolov merged commit 0e08cc6 into master Dec 16, 2016

MMatten mentioned this pull request Mar 9, 2017

Support stored and function parameters in Derby #527

Closed

javornikolov changed the title ~~Support stored and function parameters in Derby~~ Support for executing stored procedures and functions in Derby Jan 28, 2018

javornikolov added the enhancement label Jan 28, 2018

javornikolov added this to the 4.0.0 milestone Jan 28, 2018


		abstract protected Direction columnTypeToDir(int columnType);

		protected Direction getParamDirection(ResultSet rs) throws SQLException {

Support for executing stored procedures and functions in Derby #524

Support for executing stored procedures and functions in Derby #524

Conversation

MMatten commented Dec 3, 2016 • edited by javornikolov

javornikolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten commented Dec 3, 2016

MMatten commented Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javornikolov Dec 4, 2016 • edited

Choose a reason for hiding this comment

javornikolov Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten Dec 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javornikolov Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javornikolov Dec 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten commented Dec 10, 2016

javornikolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMatten commented Dec 3, 2016 •

edited by javornikolov

MMatten Dec 3, 2016 •

edited

MMatten commented Dec 3, 2016 •

edited

javornikolov Dec 4, 2016 •

edited

javornikolov Dec 3, 2016 •

edited

MMatten Dec 4, 2016 •

edited

javornikolov Dec 5, 2016 •

edited

javornikolov Dec 4, 2016 •

edited