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

Fixed the construction of query for tables with schema #26

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

talkanbaev-artur
Copy link
Contributor

Hi!

There was an issue with querying the database with the schema in the table name, i.e. select * from user.user where id=$1. I expected that the full table name with schema would deconstructed. However,t he generator gave me such result:
SELECT "users.users".* FROM "users"."users" WHERE "users.users"."id"=$1 LIMIT 1;

The table name for the FROM clause was constructed as expected, however the escape function does not deconstruct the table value at all.

As a result, the query failed, with following error: pq: missing FROM-clause entry for table "users.users"
To fix this I added a small patch that deconstructs the table name for all queries with the table value set.

@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #26 (0cfb3e6) into main (8603fe0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #26   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          604       614   +10     
=========================================
+ Hits           604       614   +10     
Impacted Files Coverage Δ
builder/buffer.go 100.00% <100.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 8603fe0...0cfb3e6. Read the comment docs.

builder/buffer.go Outdated Show resolved Hide resolved
if value == "*" {
if table == "" {
return value
}
return b.Quoter.ID(table) + ".*"
return escaped_table + ".*"
Copy link
Member

Choose a reason for hiding this comment

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

hmm, maybe we should this field to escapeCache as well, so escape_table doesn't get recomputed everytime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think we store it already.

sql/builder/buffer.go

Lines 162 to 169 in 56e8dba

result := strings.Join(parts, ".")
if len(parts) == 1 && table != "" {
result = escaped_table + "." + result
}
escapedValue = result
}
escapeCache.Store(key, escapedValue)

So I just changed the order for the cache check. If it is done before the computation of the escaped_table, then the key of table/value/quoter reuses the result of escaped value.

Or did you mean that it would be beneficial just to have a separate cache for table names?

Copy link
Member

Choose a reason for hiding this comment

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

So I just changed the order for the cache check

this is what I mean

Copy link
Member

Choose a reason for hiding this comment

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

btw, above line should call escapeCache.Store(key, escapedValue) first before returning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... I think it does call the cache store before the return

Copy link
Member

Choose a reason for hiding this comment

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

looks like it still not updated to store cache before returning? so actually the cache will never be stored

also, I think we can merge this if with below if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry - I misunderstood you. I merged statements, and know "table.*" are cached too

Copy link
Member

@Fs02 Fs02 left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you

@Fs02 Fs02 merged commit 4c61d06 into go-rel:main Apr 11, 2022
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