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/prepared statement #1960

Merged
merged 10 commits into from
Oct 15, 2020
Merged

Fix/prepared statement #1960

merged 10 commits into from
Oct 15, 2020

Conversation

IvanHdzC
Copy link
Contributor

This PR changes PreparedStatement behaviour.
Before this PR the final statement was builded with native sql driver methods, but it's failing when inserting some SQL objects. The ones identified are Timestamp with TZ and functions.(An issue was created to take a deeper look at this miss behaviour #1959)
With this PR the SQL query is builded as a plain String (as it does usually on the insert query).

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -581,12 +581,21 @@ public void upsertTransaction (LinkedHashMap<String, ArrayList<JsonElement>> agg
String insertQuery = SQLQueryUtils.sqlInsertQuery(aggregation,
Copy link
Member

Choose a reason for hiding this comment

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

CNR entry (with reference to issue number, if we have some) should be included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2f9140f

insertStatement = connection.prepareStatement(insertQuery);
/*

FIXME
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this FIXME should mention an issue number? Is this related with #1959?

Copy link
Member

Choose a reason for hiding this comment

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

(Same in the other FIXME below)

Copy link
Contributor Author

@IvanHdzC IvanHdzC Oct 15, 2020

Choose a reason for hiding this comment

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

Fixed in 6405c46

Copy link
Collaborator

@manucarrace manucarrace left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit 953dc1c into master Oct 15, 2020
@fgalan fgalan deleted the fix/prepared_statement branch October 15, 2020 13:51
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.

4 participants