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 sending json with execute #11

Merged
merged 2 commits into from
Aug 22, 2021
Merged

Conversation

hpoul
Copy link
Contributor

@hpoul hpoul commented Aug 22, 2021

I tried to bind a json value but couldn't get it to work. After further inspection it looks like json only works for query but not for execute.

Because of some reason query uses native binding, while execute uses sendSimple

if (q.onlyReturnAffectedRowCount) {
q.sendSimple(connection!._socket!);
return _PostgreSQLConnectionStateBusy(q);
}

I don't understand at all why there is a difference? Why use sendSimple for execute?

This PR is only to reproduce the bug. Please advise how to fix it? Does it make sense to try to support json in sendSimple or should everything use sendExtended?

the error message is:

PostgreSQLSeverity.error 42601: syntax error at or near "{" 

(I assume the CI should show that as well)

@isoos
Copy link
Owner

isoos commented Aug 22, 2021

I think it is a good idea to have a test for it, and this issue should be fixed, however, it may not be a good idea to commit a test to CI that always fails. Let's add a skip: true to the test method, and some comments on why this test should pass (basically the info is already in the PR, but maybe better to copy-paste it into the test).

@hpoul hpoul force-pushed the json_with_execute branch 2 times, most recently from ab44e2d to c583de3 Compare August 22, 2021 16:54
@hpoul hpoul force-pushed the json_with_execute branch from c583de3 to e2d6a83 Compare August 22, 2021 16:58
@hpoul
Copy link
Contributor Author

hpoul commented Aug 22, 2021

@isoos ok I think I found a pretty easy fix after all 😅️

but the only reason I could come up with the difference between query and execute is that "simple queries" don't support multiple queries in one message..

https://www.postgresql.org/docs/current/protocol-flow.html#:~:text=The%20query%20string%20contained,complicate%20the%20protocol%20unduly.

The query string contained in a Parse message cannot include more than one SQL statement; else a syntax error is reported. This restriction does not exist in the simple-query protocol, but it does exist in the extended protocol, because allowing prepared statements or portals to contain multiple commands would complicate the protocol unduly.

Since this is actually a use case I use myself, I think it makes sense to have both options 😅️ (Unless the client library itself parses the message good enough to split queries).

But maybe it would make sense to have the option to do it nonetheless.. I see no reason why execute statements can't be cached (ie. prepared statements). maybe we could add a allowReuse to the execute method which switches to extended queries? Anyway.. that's obviously a separate issue 😅️

@hpoul hpoul changed the title create test failure for sending json with execute fix sending json with execute Aug 22, 2021
@isoos
Copy link
Owner

isoos commented Aug 22, 2021

Fantastic finding! Thanks!

@isoos isoos merged commit 6ba2468 into isoos:master Aug 22, 2021
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.

2 participants