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.

Sign up for GitHub

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

Jump to bottom

Alias issue #70

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

Alias issue #70

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d596c12
Alias issue #69
Apr 16, 2024
6f457cc
Revert changes for Select
fairking Apr 18, 2024
7ea04ad
Alias issue
fairking Apr 18, 2024
e498e85
Alias issue
fairking Apr 18, 2024
512c4ae
Alias issue #70 (update escapeSchema method name)
fairking Apr 19, 2024
82fb599
Alias issue (remove non as aliases)
fairking Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions builder/buffer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,19 @@ func (b *Buffer) WriteEscape(value string) {
b.WriteString(b.escape("", value))
}

func (b Buffer) escape_schema(table string) string {
Copy link
Member

Choose a reason for hiding this comment

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

please change to escapeSchema

Copy link
Author

Choose a reason for hiding this comment

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

Done

if b.AllowTableSchema && strings.IndexByte(table, '.') >= 0 {
parts := strings.Split(table, ".")
for i, part := range parts {
part = strings.TrimSpace(part)
parts[i] = b.Quoter.ID(part)
}
return strings.Join(parts, ".")
} else {
return b.Quoter.ID(strings.ReplaceAll(strings.TrimSpace(table), ".", "_"))
}
}

func (b Buffer) escape(table, value string) string {
if table == "" && value == "*" {
return value
Expand All @@ -134,17 +147,11 @@ func (b Buffer) escape(table, value string) string {
var escaped_table string
if table != "" {
if i := strings.Index(strings.ToLower(table), " as "); i > -1 {
return b.escape(table[:i], "") + " AS " + b.Quoter.ID(table[i+4:])
}
if b.AllowTableSchema && strings.IndexByte(table, '.') >= 0 {
parts := strings.Split(table, ".")
for i, part := range parts {
part = strings.TrimSpace(part)
parts[i] = b.Quoter.ID(part)
}
escaped_table = strings.Join(parts, ".")
escaped_table = b.escape_schema(table[:i]) + " AS " + b.Quoter.ID(strings.TrimSpace(table[i+4:]))
} else if i := strings.Index(strings.ToLower(table), " "); i > -1 {
Copy link
Member

Choose a reason for hiding this comment

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

this will break if the table name contains spaces

Copy link
Author

@fairking fairking Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean "table name contains spaces"? Is it possible?

I just wanted to avoid any issues if someone adds extra spaces like From("books b") or From("books as b").
See TrimSpace in b.Quoter.ID(strings.TrimSpace(table[i+4:])).

Copy link
Author

@fairking fairking Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I think I know what you are talking about.
} else if i := strings.Index(strings.ToLower(table), " "); i > -1 {

So you think someone could have something like:

  1. .From("my books")
  2. .From("my books b") // where b is an alias
  3. .From("my books AS b")
  4. .From("[my books] b")

I have never saw any examples where table name contains spaces in my lifetime. And I am not sure if such would properly work in any ORM system.

But it is a good point and I am not quite sure how we can identify it. I mean the examples 1 and 2 have no indication whether it is an alias or a table name. It could be either this [my] AS [books] or this [my books]

Copy link
Author

Choose a reason for hiding this comment

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

Or you think we should remove that line and force everyone to use AS? So the syntax .From("books b") would not be valid.

Copy link
Author

@fairking fairking Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But based on probability people would want to write non space table queries more often rather then those who use spaces. The example with spaces is just some joke. You might as someone if they could define a variable with space:

var my book = rel.From("books")

This is just my opinion. Once we allow to have spaces in table names, we are doomed.

Copy link
Author

@fairking fairking Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Who would convert the table name into the variable? Struct? Sorry we must keep some consistency.
It smells like fire to me.

You are talking about something which does not exist, and it shout not exists in back end world.

Please think about the rules of the backend, it must be strong and consistent.

Let me know if I need to change anything in the code.

Copy link
Author

Choose a reason for hiding this comment

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

Are we forcing people write AS everywhere because of space in the table name?

Copy link
Author

Choose a reason for hiding this comment

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

ok, please have a look at my last commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying we should force user to use AS, but we need to find a good way if we want to support that

As for now, I think bug in example 2 is more critical

escaped_table = b.escape_schema(table[:i]) + " " + b.Quoter.ID(strings.TrimSpace(table[i+1:]))
} else {
escaped_table = b.Quoter.ID(strings.ReplaceAll(table, ".", "_"))
escaped_table = b.escape_schema(table)
}
}

Expand Down
16 changes: 16 additions & 0 deletions builder/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ func TestQuery_Build(t *testing.T) {
result: "SELECT `users`.* FROM `users` FOR UPDATE;",
query: rel.From("users").Lock("FOR UPDATE"),
},
{
result: "SELECT `c`.`id`,`c`.`name` FROM `contacts` `c`;",
query: rel.Select("c.id", "c.name").From("contacts c"),
},
{
result: "SELECT `c`.`id`,`c`.`name` FROM `contacts` AS `c`;",
query: rel.Select("c.id", "c.name").From("contacts as c"),
},
}

for _, test := range tests {
Expand Down Expand Up @@ -185,6 +193,14 @@ func TestQuery_Build_ordinal(t *testing.T) {
result: "SELECT \"users\".* FROM \"users\" FOR UPDATE;",
query: rel.From("users").Lock("FOR UPDATE"),
},
{
result: "SELECT \"c\".\"id\",\"c\".\"name\" FROM \"contacts\" \"c\";",
query: rel.Select("c.id", "c.name").From("contacts c"),
},
{
result: "SELECT \"c\".\"id\",\"c\".\"name\" FROM \"contacts\" AS \"c\";",
query: rel.Select("c.id", "c.name").From("contacts as c"),
},
}

for _, test := range tests {
Expand Down
Loading