Negative values shown for parameter with null values about datasource-proxy HOT 19 CLOSED

Hi @yeomanLearner
Thanks for the bug report.

I think the reason is the method signature for setNull is:

setNull(String parameterName, int sqlType)

Currently, datasource-proxy uses second argument for all set~ methods as parameter values.
In this case sqlType is the second argument. Thus, most likely "-5” is coming from sqlType specified when setNull is called. (it seems Types.BIGINT has value -5).

Let me think about how to handle setNull.
Probably ideal logging should show something like:
“param”=“NULL(BIGINT)” or just "param"="NULL"

Thanks,

from datasource-proxy.

Looking into the source code, it seems to the parameters map in PreparedStatementProxyLogic class is being used for two reasons

1. Method invocation
2. Logging parameters

Assuming this, it would not be right to add a special check for setNull method here, right? Also there could be more PreparedStatement or CallableStatement methods where this issue could seen?

One suggestion would be to add another method in ParameterSetOperation class called getArgsforLogging() that internally checks if the method name is "setNull", if so we return the second argument as null, so that the parameter value is correctly logged as null.

What do you think ?

from datasource-proxy.

Hi @shahamit

Thanks for checking the code.

Assuming this, it would not be right to add a special check for setNull method here, right? Also there could be more PreparedStatement or CallableStatement methods where this issue could seen?

Yes, that's correct.

One suggestion would be to add another method in ParameterSetOperation class called getArgsforLogging() that internally checks if the method name is "setNull", if so we return the second argument as null, so that the parameter value is correctly logged as null.

Yes, I think it's a great idea.

The current code for treating parameters are not so clean especially using many maps and lists...

Ideally, the logic of how to use query-args needs to be delayed until when it writes logs in DefaultQueryLogEntryCreator instead of in PreparedStatementProxyLogic.
Currently QueryInfo only holds query-string and first value of query args. I think this should be changed to hold list of ParameterSetOperation. So that, all information is available for logging to make decision how to represent the value.

I think I'll take your suggested approach in ParameterSetOperation for quick fix, and release as v1.3.1.
I'll revisit this for next major version v1.4 to pass along ParameterSetOperation for logging.

Thanks,

from datasource-proxy.

Yes, sounds great. I look forward for v1.3.1 and will integrate it as soon as it is available. We are planning to use this library in production for enabling query logging at runtime.

Eagerly waiting for the quick fix :)

from datasource-proxy.

We tried the above logic. It worked for input parameters but didn't work for output parameters. Looking at the code, it seems tricky to capture the returned values of the output parameters in case of a stored procedure call.

What are your thoughts?

from datasource-proxy.

Currently, getXxx for retrieving returned value from CallableStatement is not supported.
To solve the issue, I can modify ExecuteInfo to hold original statement(Statement, PreparedStatement, CallableStatement). So that, output parameters can be retrieved from ExecutionInfo in QueryExecutionListener#afterQuery. (there may be some db implementation limitation to retrieve results multiple times from callable-statement (in listener and in application).

What do you think?

from datasource-proxy.

I changed ExecuteInfo to hold "statement/prepared/callable" in ef19117.

I'm thinking to release v1.3.1 end of this week.
Let me know if you have something else wants to be added.

from datasource-proxy.

Looks good. Based on your suggestions above we are trying to see if output parameter values could be logged. It seems there are quite a number of changes in various classes.

In case it works, it would be great to have it in the library code, but as you said there might be some db implementation limitation to retrieve results multiple times.
I didn't spend time in understanding the current code design to the fine detail, but do you think we could add such code and enable it if the underlying datasource is Oracle. Clients can enable the listener only when they know what they are doing.

What do you think?

from datasource-proxy.

In current implementation(v1.3 or 1.3.1), listener(ExecutionInfo) cannot know whether params are for input or output.
If you can get to know which parameters are for outputs based on query, parameter name, or something else, you can retrieve output parameters.
For example, if your output parameter names are always start with "output", then you can filter those names from QueryInfo#getQueryArgsList().get(...).keySet() and retrieve outputs from callable statement(ExecutionInfo#getStatement()(v1.3.1)).

In v1.4, I'll change ExecutionInfo to keep ParameterSet which contains the invoked methods for setting parameters. So that, it'll be possible to distinguish which params are for input or output by comparing method name("registerOutParameter") for each parameter.

Adding database specific implementation is an interesting idea. I'll think about it, but at the same time, if you write such a listener to get extra information, pull request is more than welcome.

from datasource-proxy.

We tried out the implementation and here are the points we noticed

1. As you said in your last comment, yes we would need to distinguish between input and output parameters, but there is a catch. We would need to further distinguish between output parameters that are registered by index vs those that are registered by name. registerOutParameter method is overloaded. While logging, we need to call the right getOutput method.
2. In one of your earlier replies you mentioned that some db implementations might have a limitation on retrieving results multiple times. Did you refer to the preparedStatement.getResultSet() method call? When we tried it with Oracle, we had call preparedStatement.getObject() method instead. getResultSet() method does return a null when called multiple times (as the javadoc also claims) but that's not what we had to call. So did you mean that preparedStatement.getObject() method call would also fail for other db implementation when called multiple times?

Currently for logging output parameter value we are modified existing classes. We will see how can we extract it to a listener and reuse the current logging code at the same time. Once we are able to achieve that, we would make a pull request.

Thanks !

from datasource-proxy.

1. As you said in your last comment, yes we would need to distinguish between input and output parameters, but there is a catch. We would need to further distinguish between output parameters that are registered by index vs those that are registered by name. registerOutParameter method is overloaded. While logging, we need to call the right getOutput method.

Yes, registerOutParameter has both String parameterName and int parameterIndex.
I'm planning to update QueryInfo to hold list of ParameterSetOperation which contains java.lang.reflect.Method and args used for setting parameters. From Method, it can check the parameter types of the arguments, so that you can distinguish int or String. Or, you can use method args in conjunction with method to find int or String.

1. In one of your earlier replies you mentioned that some db implementations might have a limitation on retrieving results multiple times. Did you refer to the preparedStatement.getResultSet() method call? When we tried it with Oracle, we had call preparedStatement.getObject() method instead. getResultSet() method does return a null when called multiple times (as the javadoc also claims) but that's not what we had to call. So did you mean that preparedStatement.getObject() method call would also fail for other db implementation when called multiple times?

I meant for get... methods in general. I don't think JDBC spec defines the behavior of calling data retrieval methods multiple times (need to check). But I also think if it can log outputs, that would be certainly useful. As long as displaying outputs are configurable(maybe default is off for being conservative), user can choose what to show.

I am planning to change the master branch for next major release 1.4.

• Maven multi module layout
• New module for unittest
• Updates involving API changes (includes ParameterSetOperation change mentioned above)
• etc.

I have created 1.3.x branch to keep current code base easier.
If you are going to create a PR based on the current code, you can send it to 1.3.x branch. Then, I'll think about merging to master branch.
Or, you can follow master branch. Whichever easy for you.

Thanks for many feedbacks!!

from datasource-proxy.

Your plan sounds interesting.

About logging output parameters, now that we see it working I was looking how to integrate it with rest of the code. As you said it would have to be configurable. I thought about implementing it as part of the listener but that seems to lead to a lot of code duplication. The listener would be configured only when output parameters are to be logged since it might just work of some databases (Oracle is what we know off). Code duplication because we would have to repeat all the code that logs other values - name, query, execution time, input parameters etc.
One thought would be allow plugins to the listeners. The listener would call the plugin to suffix any other logs after the standard query logging is done (passing the handle to the StringBuilder?).

What do you think?

from datasource-proxy.

Great to hear that logging output parameters is working!!

Listener plugin sounds to be a little bit over engineering for me.
Since QueryLogEntryCreator is constructing the log entry string, can it be another implementation extending the default one, and inject it to listener?

from datasource-proxy.

I agree that listener plugin would be over engineering but creating another implementation of QueryLogEntryCreator would also mean duplicating a lot of code that logs other details - name, query, execution time, input parameter values etc, right?

from datasource-proxy.

yeah, ideally the default impl can be more extendable. but if it is appending new output, can it be something like:

class MyCreator extends DefaultQueryLogEntryCleator {
  @Override
  public String getLogEntry(...) {
      StringBuilder sb = ...
      sb.append(super.getLogEntry(...));
      ...  // add more logging
      return sb.toString();
  }
}

from datasource-proxy.

Yeah, we can work out this way too. An additional change which would be need along with this is that the PreparedStatementProxyLogic class would have to exclude output parameters from the queryInfo.getQueryArgsList(), otherwise they would be logged twice with incorrect values initially.

from datasource-proxy.

Hey, we have tried out the changes we spoke in this thread and they work as expected. When do you plan to incorporate them in the library so that don't end up modifying library code?
There are only a couple of changes needed

1. Separating input and output parmeter list in PreparedStatementProxyLogic
2. Removing the code to log output parameter name with type which was added recently in v3.1. (since we can now log output parameter values)

A new listener (and a log entry creater) would then log output parameter values.

We are looking for the above two changes to be part of the library as soon as possible so that we could integrate it in our software in the next release cycle. If your plan for v4 is going to take long, it would be great if you consider having these changes in v3.2.

Thoughts?

from datasource-proxy.

Hi, I finally back from my long vacation.
Fighting with jet lag and catching up my regular work, but will find some time and check up the pull request.
Once it is merged, I'll release v1.3.2.

Thanks,

from datasource-proxy.

Hi @shahamit, @parikshitN

Based on the output parameter pull-request, I published v1.3.2 which contains CommonsOracleOutputParameterLoggingListener (renamed a bit from pull request).
Now maven central has synced, so you can download and don’t need to maintain patched version.

Thanks for the contribution!!

from datasource-proxy.