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

fix: Improve type detection with casing issues. #404

Merged
merged 1 commit into from Oct 16, 2015

Conversation

Projects
None yet
3 participants
@zapov
Contributor

zapov commented Oct 16, 2015

Instead of lowercasing typename which causes problems for types which require quote due to non lowercase casing, lowercase on failed alias check and during type lookup for types without schema or quote.

Improve the quote casing heuristics by checking for quote char.
Support case type in schemas on path without schema reference.

Current rules for casing

type => "type"
TYPE => "type"
"Type" => "Type"
NS.TYPE => "ns"."type"
"NS"."Type" => "NS"."Type"
NS."TYPE" => "ns"."TYPE"

This change introduces breaking change. Previously casing was not changed.
Now unquoted names will be considered lowercase.

So if you had type: CREATE TYPE "TIMESTAMPTZ"(at timestamp, offset int)
you could reference it with TIMESTAMPTZ. Now quotes are required, otherwise it will default to timestamptz which is Postgres built-in type.

Fixes issue with: f1a5cc4 which would interpret "Ns"."Type" as "ns"."type"

@zapov zapov force-pushed the zapov:master branch from 0eec111 to 95cd3c4 Oct 16, 2015

@@ -290,8 +285,10 @@ private PreparedStatement getOidStatement(String pgTypeName) throws SQLException
}
_getOidStatementSimple = _conn.prepareStatement(sql);
}
// coerce to lower case to handle upper case type names
String lcName = pgTypeName.toLowerCase(Locale.ENGLISH);

This comment has been minimized.

@zapov

zapov Oct 16, 2015

Contributor

I actually don't think this is a good idea, but if you want such behavior probably a better way to go about it is to register built-in types and aliases as upercase.

@davecramer

This comment has been minimized.

Member

davecramer commented Oct 16, 2015

Why is this a bad idea? Also you should realize that PostgreSQL coerces to lower case by default

@zapov

This comment has been minimized.

Contributor

zapov commented Oct 16, 2015

Of course I know Postgres defaults to lowercase ;)

I think its bad idea for two reasons:

  1. it's a breaking change
  2. it's inconsistent

So it should be either
TYPE => "type"
NS.TYPE => "ns"."type"
Ns.Type => "ns"."type"
NS."TYPE" => "ns"."TYPE"

or

TYPE => "TYPE"
NS.TYPE => "NS"."TYPE"
Ns.Type => "Ns"."Type"
NS."TYPE" => ??? probably "NS"."TYPE"

Oracle behaves the second way, at least in .NET driver (without the support for quotes)

btw. I don't mind the lowercasing (thats fine with me) I mind inconsistency... So we should also convert
Ns.Type => "ns"."type"

fix: Improve type detection with casing issues.
Instead of lowercasing typename which causes problems for types which require quote due to non lowercase casing,
lowercase on failed alias check and during type lookup for types without schema or quote.

Improve the quote casing heuristics by checking for quote char.
Support case type in schemas on path without schema reference.

Current rules for casing

TYPE => "type"
Type => "type"
"TYPE" => "TYPE"
NS.TYPE => "ns"."type"
"NS"."TYPE" => "NS"."TYPE"
NS."TYPE" => "ns"."TYPE"

Not sure about the last one and the first one.

@zapov zapov force-pushed the zapov:master branch from 95cd3c4 to 0de91a5 Oct 16, 2015

@zapov

This comment has been minimized.

Contributor

zapov commented Oct 16, 2015

I've repushed my changes to make it consistent now.
The change you've made was not good since it would convert "Ns"."Type" => "ns"."type" due to lowercase on typename.

Now
Ns.Type => "ns"."type"
"Ns"."Type" => "Ns"."Type"

This is still a breaking change, but I doubt people will run into the BC.

@vlsi

This comment has been minimized.

Member

vlsi commented Oct 16, 2015

This is still a breaking change, but I doubt people will run into the BC.

Can you put that in a short summary? (e.g. for the release notes)

Ns.Type => "ns"."type"
"Ns"."Type" => "Ns"."Type"

I do not find tests that cover that behavior.
Do they exist? Are you going to add some?

@@ -90,7 +90,7 @@ public void testCompositeFromTable() throws SQLException {
pstmt.setObject(1, pgo1);
String[] ctArr = new String[1];
ctArr[0] = "(\"{1,2}\",{},\"(1,2.2,)\")";
Array pgarr1 = _conn.createArrayOf("Composites.ComplexCompositeTest", ctArr);
Array pgarr1 = _conn.createArrayOf("\"Composites\".\"ComplexCompositeTest\"", ctArr);

This comment has been minimized.

@zapov

zapov Oct 16, 2015

Contributor

This previous test failed due to BC. But this is only allowed since 1202 (so it's not really an issue)

}

public void testCasingBuiltinAlias() throws SQLException {
Array arr = _conn.createArrayOf("INT", new Integer[] { 1 , 2, 3});

This comment has been minimized.

@zapov

zapov Oct 16, 2015

Contributor

And this test checks the new behavior (which only worked with lowercase int before)

@zapov

This comment has been minimized.

Contributor

zapov commented Oct 16, 2015

I've updated the Github commit message. Hopefully it's good enough now ;)

davecramer added a commit that referenced this pull request Oct 16, 2015

Merge pull request #404 from zapov/master
fix: Improve type detection with casing issues.

@davecramer davecramer merged commit ad7fc43 into pgjdbc:master Oct 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment