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: getPGArrayType fails in when stringType=unspecified #1036

Merged
merged 16 commits into from Jan 23, 2018

Conversation

Projects
None yet
4 participants
@JamiePullar
Copy link
Contributor

JamiePullar commented Dec 15, 2017

When resolving a type for a create array statement, such as
connection.createArrayOf("citext", myArray)
The postgres driver would throw the following exception

Caused by: org.postgresql.util.PSQLException: ERROR: could not determine data type of parameter $3
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2477)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2190)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:300)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:428)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:354)
	at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:169)
	at org.postgresql.jdbc.TypeInfoCache.getPGType(TypeInfoCache.java:368)
	at org.postgresql.jdbc.TypeInfoCache.getPGArrayType(TypeInfoCache.java:440)
	at org.postgresql.jdbc.PgConnection.createArrayOf(PgConnection.java:1277)
	at product.data.postgres.PgCitextArrayBuilder$.apply(TypeAdapters.scala:288)

this is cause by a ? IS NULL clause in the _getOidStatementComplexArray statement.
Replacing with ? - a Boolean parameter and setting with the value schema == null fixes the problem

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 15, 2017

Looking at this I think the change is good. I'd like to as a favour though.
Can you add some comments around why we check for a null schema there? It isn't immediately obvious(to me anyway)
Can you add a test case for this as it obviously wasn't covered before ?

Thanks

jamie.pullar@concentra.co.uk
@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Dec 15, 2017

I have include test cases which use types which are available in 8.2, however I have only run them against postgres 9.5

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 15, 2017

jamie.pullar@concentra.co.uk added some commits Dec 16, 2017

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Dec 16, 2017

I have include test cases

@JamiePullar, could you please clarify how your test verifies schema == null case?
I would prefer adding a regular JDBC test

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Dec 16, 2017

@vlsi by not specify a schema in the type name ie, "text[]" as opposed to something like "MySchema"."text[]", the null case is tested.

So the implemented tests in this instance actually only test the null case.
I imagine it could be possible to implement tests around the not null case by creating a schema and then executing a CREATE TYPE statement. I can add this additional test

However if you are not happy with where I placed the tests, if you could tell me the path for where they should go, and the path to a regular JDBC test so Im absolutely clear on what you want, Ill be happy to make the change.

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Dec 16, 2017

The tests you added examine implementation details of typeCache. That is a bad sign. For instance, the tests might fail to test the right thing after TypeCache refactoring.

Adding a scheme is fine. You can add a test method to an existing classes that creates schema if you find that relevant: https://github.com/pgjdbc/pgjdbc/search?utf8=%E2%9C%93&q=create+schema&type=

jamie.pullar@concentra.co.uk added some commits Dec 16, 2017

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Dec 16, 2017

I have discovered in the test case, tests which successfully test the schema null case and succeed.
Digging deeper I found the reason that the error occurs is if the connection property stringtype has been left defaulted to unspecified.
I have created a test case in ArrayTest that fails with the old code but now succeeds with fix. I hope this should sufficient, it does however introduce the need for another open connection into the test suite. I'm not entirely clear on best practice for this one.
If you think its more appropriate to ditch this pull request and raise a bug, let me know

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 16, 2017

Codecov Report

Merging #1036 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1036      +/-   ##
============================================
+ Coverage     67.26%   67.27%   +<.01%     
- Complexity     3668     3669       +1     
============================================
  Files           170      170              
  Lines         15632    15632              
  Branches       2527     2528       +1     
============================================
+ Hits          10515    10516       +1     
+ Misses         3930     3928       -2     
- Partials       1187     1188       +1
jamie.pullar@concentra.co.uk
@@ -53,6 +56,10 @@ public void setUp() throws Exception {
super.setUp();
_conn = con;

Properties props = new Properties();
PGProperty.STRING_TYPE.set(props, "unspecified");
_stringTypeUnspecifiedConn = TestUtil.openDB(props);

This comment has been minimized.

@vlsi

vlsi Dec 17, 2017

Member

Please move that to its own test class and use BaseTest4.updateProperties:

protected void updateProperties(Properties props) {

jamie.pullar@concentra.co.uk added some commits Dec 17, 2017

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 18, 2017

FYI you can run maven checkstyle:check locally before pushing to github

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Dec 18, 2017

@davecramer Ah this is my first time with maven. Thanks will do from now :)

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 18, 2017

psql adds a test for isvisble as well.

\dT bool
********* QUERY **********
SELECT n.nspname as "Schema",
  pg_catalog.format_type(t.oid, NULL) AS "Name",
  pg_catalog.obj_description(t.oid, 'pg_type') as "Description"
FROM pg_catalog.pg_type t
     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace
WHERE (t.typrelid = 0 OR (SELECT c.relkind = 'c' FROM pg_catalog.pg_class c WHERE c.oid = t.typrelid))
  AND t.typname !~ '^_'
  AND (t.typname ~ '^(bool)$'
        OR pg_catalog.format_type(t.oid, NULL) ~ '^(bool)$')
  AND pg_catalog.pg_type_is_visible(t.oid)
ORDER BY 1, 2;
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jan 12, 2018

@JamiePullar any chance you can address the changes requested ?

@vlsi vlsi force-pushed the pgjdbc:master branch from 4a6d1d1 to 5819f31 Jan 17, 2018

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Jan 21, 2018

Hello, yes sorry @davecramer, am I not seeing something here? Im not sure whats still outstanding.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jan 21, 2018

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Jan 21, 2018

@davecramer , I see @vlsi comment 'Please move that to its own test class and use BaseTest4.updateProperties:' which I had subsequently done. Is there something I'm missing, apologies for being a bit thick here?

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jan 21, 2018

@JamiePullar

This comment has been minimized.

Copy link
Contributor

JamiePullar commented Jan 21, 2018

No worries, and thanks for all your time on this :)

import java.util.Properties;

@RunWith(Parameterized.class)
public class StringTypeUnspecifiedArrayTest extends BaseTest4 {

This comment has been minimized.

@vlsi

vlsi Jan 21, 2018

Member

Please add the test in question to the relevant test suite

This comment has been minimized.

@JamiePullar

JamiePullar Jan 21, 2018

Contributor

@vlsi sure thing, ummm, what is the relevant test suite?

This comment has been minimized.

@vlsi

vlsi Jan 21, 2018

Member

org.postgresql.test.jdbc2.Jdbc2TestSuite

@vlsi vlsi added this to the 42.2.1 milestone Jan 21, 2018

@@ -0,0 +1,49 @@
/*
* Copyright (c) 2007, PostgreSQL Global Development Group

This comment has been minimized.

@vlsi

vlsi Jan 21, 2018

Member

2018

* See the LICENSE file in the project root for more information.
*/

package org.postgresql.test.jdbc4;

This comment has been minimized.

@vlsi

vlsi Jan 21, 2018

Member

Move to org.postgresql.test.jdbc2, please

jamie.pullar@concentra.co.uk and others added some commits Jan 22, 2018

jamie.pullar@concentra.co.uk

@vlsi vlsi force-pushed the HigherState:master branch from e639432 to 0c00027 Jan 23, 2018

@vlsi vlsi changed the title Null value breaking query in getOidStatement, fix fix: getPGArrayType fails in when stringType=unspecified Jan 23, 2018

@vlsi vlsi merged commit d5f1cf7 into pgjdbc:master Jan 23, 2018

2 checks passed

codecov/project 67.27% (+<.01%) compared to e133510
Details
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