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

Dynamic Queries #2859

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Dynamic Queries #2859

wants to merge 14 commits into from

Conversation

ovadbar
Copy link
Contributor

@ovadbar ovadbar commented Oct 15, 2023

This is an attempt at my proposal for sqlc dynamic queries. (It also fixes an issue regarding table scope that I want).

// If the query string was edited, make sure the syntax is valid
if expanded != rawSQL {
if _, err := c.parser.Parse(strings.NewReader(expanded)); err != nil {
return nil, fmt.Errorf("edited query syntax is invalid: %w", err)
return nil, fmt.Errorf("edited query syntax is invalid: %w - %s", err, expanded)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to help me debug I could remove if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move this pgx directory into a subdirectory with the stdlib directory?

@@ -1,3 +1,4 @@
{
"process": "sqlc-gen-json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't have sqlc-gen-json in my path so I added this to get the tests to pass I can remove it.

"dollar": func() bool {
return req.Settings.Engine == string(config.EnginePostgreSQL)
},
"hasDynamic": func() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how to add these values without putting them here. So while it does work it is kind of a hack.

@andrewmbenton
Copy link
Collaborator

We are unlikely to merge anything related to "dynamic" SQL until we have come up with a proposal internally.

That said, I think we could look at the issue you mentioned related to table scope. Is there an open issue in the issue tracker for that?

@ovadbar
Copy link
Contributor Author

ovadbar commented Oct 17, 2023

We are unlikely to merge anything related to "dynamic" SQL until we have come up with a proposal internally.

Yeah I contributed to the dynamic discussion and decided to create this pull request in order to showcase how it would work.

That said, I think we could look at the issue you mentioned related to table scope. Is there an open issue in the issue tracker for that?
You already looked at it it is this pull request #2639

out = append(out, "pq.Array("+escape(v.Name)+")")
} else {
out = append(out, escape(v.Name))
}
} else {
for _, f := range v.Struct.Fields {
if !f.HasSqlcSlice() && strings.HasPrefix(f.Type, "[]") && f.Type != "[]byte" && !v.SQLDriver.IsPGX() {
if f.HasSqlcDynamic() {

Choose a reason for hiding this comment

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

Hi @ovadbar there is no logic inside this condition. May be it is not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I have should have written it as the following. The code is needed

if !f.HasSqlcDynamic() {
    if !f.HasSqlcSlice() && strings.HasPrefix(f.Type, "[]") && f.Type != "[]byte" && !v.SQLDriver.IsPGX() {
	out = append(out, "pq.Array("+escape(v.VariableForField(f))+")")
    } else {
	out = append(out, escape(v.VariableForField(f)))
    }
}

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.

3 participants