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

Support Informix #436

Closed
wants to merge 215 commits into from
Closed

Support Informix #436

wants to merge 215 commits into from

Conversation

MMatten
Copy link
Contributor

@MMatten MMatten commented Oct 25, 2015

Support for Informix (using alternate approach of allowing StatementExecution sub-classing instead of adding more parameters to existing class methods and more property getters on the DBEnvironment interface).

TODO:

  • Simplify testing expected exceptions
  • Test the class of the Big Decimal transform() output. (The reason which forced us to have custom handling for Informix).
  • Decide about the big decimal to exact java.math.BigDecimal transformer name
  • Some refactoring - db accessor set method
  • DbParameterAccessor: select the name of the transform to JDBC - compatible type method
  • Rename JDBC to Jdbc for the newly created fields and classes

MMatten and others added 30 commits October 3, 2015 21:33
… still not working)

-moved setAutoCommit to AbstractDbEnvironment.afterConnectionEstablished
-added calls to getAutoCommit before calling setAutoCommit
-removed overrides to commit / afterConnectionEstablished in InformixEnvironment
paramDirection, getSqlType(dataType), getJavaClass(dataType),
paramPosition, normaliseTypeName(dataType), userTypeName);

return dbp;
}

private OracleDbParameterAccessor createOracleDbParameterAcccessor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work if we just override createDbParameterAcccessor here? (Just to be keep things like in other envs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can overload createDbParameterAcccessor (the Oracle env method has the extra String userTypeName param) just to give this the same name as in AbstractDbEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's rename and add @Override annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename/overload but not add the annotation as we're not overriding the super-type's method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we have different list of parameters. Then I think it would be better to keep it as it is - with Oracle in the name (just to make it more obvious that it's a bit different).

@@ -245,7 +247,8 @@ public Class getJavaClass(String dataType) {
if (!dataType.equals("void")) {
allParams.put("", new DbParameterAccessor("",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here - createDbParameterAccessor vs new ...

@@ -1,5 +1,6 @@
package dbfit.util;

import dbfit.util.TypeTransformer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this import is also unused.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 1, 2015

Ok. All applied.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 1, 2015

Ah, need to revert that overloading!

@@ -14,9 +14,9 @@
return parameterAccessors;
}

public void add(String name, Direction direction, int sqlType, Class javaType) {
public void add(String name, Direction direction, int sqlType, Class javaType, TypeTransformerFactory dbfitToJdbcTransformerFactory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TypeTransformerFactory dbfitToJdbcTransformerFactory may have been moved as attribute (and initialized via the constructor) instead of passing it as parameter.
But no urgency to tweak that. Can be revised some other time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just about to push a commit to change this...

@javornikolov
Copy link
Contributor

I think we're already in relatively good shape here. As we proceed towards merging the stuff:

  • Maybe it makes sense to push some of the refactoring (the support of adding toJdbcCompatible transformers) as separate PR? It may be helpful to review it later for further improvements (or if some regression emerges /though I hope we don't have any/).
  • Also consider squashing the commits.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

Maybe it makes sense to push some of the refactoring (the support of adding toJdbcCompatible transformers) as separate PR?

Shall we try to slice this off and merge it before the main Informix changes?

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

Shall we try to slice this off and merge it before the main Informix changes?

If "yes", then can you give me a few pointers on how to do that?

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

@javornikolov
Copy link
Contributor

Shall we try to slice this off and merge it before the main Informix changes?
If "yes", then can you give me a few pointers on how to do that?

Yes. If we're about to squash the commits - then one approach is:

  • create a new branch from master
  • git merge --squash --no-commit
    Then remove the files not related to what we're moving there (git checkout -- path). And then commit.

If history is about to be preserved: some variation of git rebase can be used to select which commits we want to move.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

We've touched AbstractDbEnvironment and DBEnvironment for both the 'JDBC compatible value' change and the change to create the StatementExecution sub-types (required for the Informix stuff).

I guess I'll have to go looking for the commit where this direction (the 2nd change) was started. Or do you know another way of approaching this?

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

Sorry, make that just AbstractDbEnvironment.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

The docs for the --no-squash variant say it commits so no chance of even using rebase -i.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

I guess I can just undo that change on that one file and deal with the conflict later when merging the Informix stuff.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

...and DbParameterAccessor.

@javornikolov
Copy link
Contributor

We've touched AbstractDbEnvironment and DBEnvironment for both the 'JDBC compatible valuechange and the change to create theStatementExecution` sub-types (required for the Informix stuff).

I guess I'll have to go looking the commit where this direction was started. Or do you know another way of approaching this?

Right... I've forgotten about that create statement execution. Makes sense to go with separate PR for it too. So just merge --squash would be too bulky perhaps. Interactive rebase might be the easiest way to go.

@MMatten
Copy link
Contributor Author

MMatten commented Dec 2, 2015

Part one created on #448.

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

Successfully merging this pull request may close these issues.

None yet

3 participants