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

Fix JDBC fatal error #69

Merged
merged 3 commits into from
Jul 2, 2020
Merged

Fix JDBC fatal error #69

merged 3 commits into from
Jul 2, 2020

Conversation

koxudaxi
Copy link
Owner

@koxudaxi koxudaxi commented Jun 18, 2020

This PR fixes unexpected JDBC fatal error when JDBC gets an error.

The bug depends on alpine and a version of jpype.
I will merge this PR when jpype will be released

Related Issues

#50
jpype-project/jpype#763

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #69 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #69   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          648       691   +43     
  Branches        78        87    +9     
=========================================
+ Hits           648       691   +43     
Impacted Files Coverage Δ
local_data_api/resources/jdbc/__init__.py 100.00% <100.00%> (ø)
local_data_api/resources/jdbc/mysql.py 100.00% <100.00%> (ø)
local_data_api/resources/jdbc/postgres.py 100.00% <100.00%> (ø)
local_data_api/models.py 100.00% <0.00%> (ø)
local_data_api/secret_manager.py 100.00% <0.00%> (ø)
local_data_api/resources/resource.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e46d87...4b76680. Read the comment docs.

@MarkHerhold
Copy link
Contributor

MarkHerhold commented Jun 30, 2020

I believe this PR (combined with master) messes up boolean types. I checked, and master works just fine.

Basically this PR makes the value "longValue": 1 where it should be "booleanValue": true

Query:

SELECT 1 > 0 AS value;
+---------+
| value   |
|---------|
| True    |
+---------+
SELECT 1
Time: 0.019s

Reproduce:

export RDS_DATA_API_CLIENT_RESOURCE_ARN=arn:aws:rds:us-east-1:123456789012:cluster:dummy
export RDS_DATA_API_CLIENT_SECRETARN=arn:aws:secretsmanager:us-east-1:123456789012:secret:dummy
aws --endpoint-url 'http://localhost:8080' rds-data execute-statement --database 'test' --resource-arn $RDS_DATA_API_CLIENT_RESOURCE_ARN --secret-arn $RDS_DATA_API_CLIENT_SECRETARN --sql 'SELECT 1 AS hi'

On koxudaxi/local-data-api:0.4.13:

      executeStatement({
        secretArn: 'arn:aws:secretsmanager:us-east-1:123456789012:secret:dummy',
        resourceArn: 'arn:aws:rds:us-east-1:123456789012:cluster:dummy',
        database: 'test',
        includeResultMetadata: true,
        parameters: [ [length]: 0 ],
        sql: 'SELECT 1 > 0 AS value'
      })

    console.log
      {
        "columnMetadata": [
          {
            "arrayBaseColumnType": 0,
            "isAutoIncrement": false,
            "isCaseSensitive": false,
            "isCurrency": false,
            "isSigned": false,
            "label": "value",
            "name": "value",
            "nullable": 2,
            "precision": 1,
            "scale": 0,
            "tableName": "",
            "type": -7,
            "typeName": "bool"
          }
        ],
        "numberOfRecordsUpdated": 0,
        "records": [
          [
            {
              "booleanValue": true
            }
          ]
        ]
      }

On #69 with master branch merged in:

      executeStatement({
        secretArn: 'arn:aws:secretsmanager:us-east-1:123456789012:secret:dummy',
        resourceArn: 'arn:aws:rds:us-east-1:123456789012:cluster:dummy',
        database: 'test',
        includeResultMetadata: true,
        parameters: [ [length]: 0 ],
        sql: 'SELECT 1 > 0 AS value'
      })

    console.log
      {
        "columnMetadata": [
          {
            "arrayBaseColumnType": 0,
            "isAutoIncrement": false,
            "isCaseSensitive": false,
            "isCurrency": false,
            "isSigned": false,
            "label": "value",
            "name": "value",
            "nullable": 2,
            "precision": 1,
            "scale": 0,
            "tableName": "",
            "type": -7,
            "typeName": "bool"
          }
        ],
        "numberOfRecordsUpdated": 0,
        "records": [
          [
            {
              "longValue": 1
            }
          ]
        ]
      }

@koxudaxi
Copy link
Owner Author

koxudaxi commented Jul 1, 2020

@MarkHerhold
Thank you for your report.
I research the bug.

JPype1 change to return type of Java from SQL results in a newer version.

I add handling actual JDBC type for DataAPI.

Also, JPype1 doesn't support the latest jpype.
I change base image alpine to Debian. the image size will be large.
I don't afraid of the size because this image don't be used as production.

@koxudaxi
Copy link
Owner Author

koxudaxi commented Jul 1, 2020

image size:
This PR -> 562MB
0.4.15 -> 239MB

@MarkHerhold
Copy link
Contributor

@koxudaxi I think the size increase is completely fine, especially for a development image such as this 👍

@MarkHerhold
Copy link
Contributor

I may have been mistaken with my bug report because MySQL's bool is 0/1 and Postgres' bool is true/false 😟

@koxudaxi
Copy link
Owner Author

koxudaxi commented Jul 2, 2020

@MarkHerhold
Could we merge this PR into master branch?
How do you think about it?

@MarkHerhold
Copy link
Contributor

MarkHerhold commented Jul 2, 2020

I'd merge master into this first, ensure CI works, then merge this PR.
I can test a bit more after master is merged into this branch if you like.

@koxudaxi koxudaxi merged commit 83d6d54 into master Jul 2, 2020
@koxudaxi koxudaxi deleted the fix_JDBC_fatal_error branch July 2, 2020 17:49
@koxudaxi
Copy link
Owner Author

koxudaxi commented Jul 2, 2020

@MarkHerhold
The integration test passed in this PR.
I have merged the PR.

Would you test the master ?
If we don't see any problem, then I will publish a new image to the docker hub this weekend.

Thank you very much.

@koxudaxi koxudaxi changed the title [WIP] fix JDBC fatal error fix JDBC fatal error Jul 2, 2020
@koxudaxi koxudaxi changed the title fix JDBC fatal error Fix JDBC fatal error Jul 2, 2020
@MarkHerhold
Copy link
Contributor

@koxudaxi Everything worked well for me on master 👍

@koxudaxi
Copy link
Owner Author

koxudaxi commented Jul 4, 2020

@MarkHerhold

I released a new version as 0.5.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

2 participants