Skip to content
This repository has been archived by the owner on Oct 17, 2020. It is now read-only.

Added GetByAliases function in db and repository #539

Merged
merged 21 commits into from Mar 12, 2020
Merged

Added GetByAliases function in db and repository #539

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0cc18b3
Added GetByAliases function in db and repository
Mar 9, 2020
5071836
Merge branch 'master' into tom/repo
dawntomm Mar 9, 2020
c42e35e
Added GetByAliases function in db and repository
Mar 9, 2020
38de94e
added unit test
Mar 9, 2020
3d3fd2f
Merge branch 'tom/repo' of https://github.com/dawntomm/short into tom…
Mar 9, 2020
3bb21a3
rewrote GetByAliases logic
Mar 10, 2020
0bc2956
Merge branch 'master' into tom/repo
dawntomm Mar 10, 2020
32f1603
Merge branch 'master' into tom/repo
coderworld10 Mar 10, 2020
44741eb
Merge branch 'master' into tom/repo
coderworld10 Mar 11, 2020
fcb8bd0
addressed comment
Mar 11, 2020
deca9ca
Merge branch 'tom/repo' of https://github.com/dawntomm/short into tom…
Mar 11, 2020
2405877
addressed comment
Mar 11, 2020
dc9d937
revert .env file changes
Mar 11, 2020
1016e42
added prepare statement
Mar 11, 2020
264c13f
Merge branch 'master' into tom/repo
coderworld10 Mar 11, 2020
e50e35a
added TODO
Mar 11, 2020
fac0bf6
Merge branch 'tom/repo' of https://github.com/dawntomm/short into tom…
Mar 11, 2020
d36a571
Update backend/app/adapter/db/url.go
coderworld10 Mar 12, 2020
09b97a0
Update backend/app/adapter/db/url.go
coderworld10 Mar 12, 2020
53f7d73
Apply suggestions from code review
coderworld10 Mar 12, 2020
480c5d1
Update backend/app/adapter/db/url.go
coderworld10 Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 74 additions & 0 deletions backend/app/adapter/db/url.go
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ package db
import (
"database/sql"
"fmt"
"strings"

"github.com/short-d/short/app/adapter/db/table"
"github.com/short-d/short/app/entity"
Expand Down Expand Up @@ -96,6 +97,79 @@ WHERE "%s"=$1;`,
return url, nil
}

// GetByAliases finds URLs for a list of aliases
func (u URLSql) GetByAliases(aliases []string) ([]entity.URL, error) {
parameterStr := u.composeParamList(len(aliases))

// create a list of interface{} to hold aliases for db.Query()
aliasesInterface := []interface{}{}
for _, alias := range aliases {
aliasesInterface = append(aliasesInterface, alias)
}

var urls []entity.URL

// TODO: compare performance between Query and QueryRow. Prefer QueryRow for readability
statement := fmt.Sprintf(`
SELECT "%s","%s","%s","%s","%s"
FROM "%s"
WHERE "%s" IN (%s);`,
dawntomm marked this conversation as resolved.
Show resolved Hide resolved
table.URL.ColumnAlias,
table.URL.ColumnOriginalURL,
table.URL.ColumnExpireAt,
table.URL.ColumnCreatedAt,
table.URL.ColumnUpdatedAt,
table.URL.TableName,
table.URL.ColumnAlias,
parameterStr,
)

stmt, err := u.db.Prepare(statement)
if err != nil {
return urls, err
}
defer stmt.Close()

rows, err := stmt.Query(aliasesInterface...)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't find the for loop and QueryRow in the PR. Please checkout this example mentioned in one of the previous articles:

db, err := sql.Open("postgres", "postgres://postgres:postgres@localhost/postgres?sslmode=disable")
	if err != nil {
		log.Fatal(err)
	}
	defer db.Close()
	stmt, err := db.Prepare("select count(*) from pg_stat_activity where datname = $1")
	if err != nil {
		log.Fatal(err)
	}
	defer stmt.Close()
	var num int
	start := time.Now()
	for i := 0; i < 1000; i++ {
		err = stmt.QueryRow("postgres").Scan(&num)
	}
	elapsed := time.Since(start)
	fmt.Printf("got: %d in %s\n", num, elapsed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to official document for function QueryRow:
// If the query selects no rows, the *Row's Scan will return ErrNoRows.
// Otherwise, the *Row's Scan scans the first selected row and discards
// the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems that the document is only selecting a single row for testing purpose.
Here we want to select a series of rows, I believe we have to use Query

Copy link
Member

Choose a reason for hiding this comment

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

Please read this whole thread: golang/go#5171

Copy link
Contributor

Choose a reason for hiding this comment

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

@byliuyang In that thread, they talk about bulk operations a way to insert multiple rows at once.

Our case is a standard SQL query, which requires to call Query and pass the list of args.
Please read this http://go-database-sql.org/prepared.html

Go creates prepared statements for you under the covers. A simple db.Query(sql, param1, param2), for example, works by preparing the sql, then executing it with the parameters and finally closing the statement.

Copy link
Member

Choose a reason for hiding this comment

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

@alldroll I want to make everything explicit to avoid magics.
@dawntomm I don't want to block your changes. Let's add a TODO here to further discuss our SQL handling strategy. I'll approve this PR after the TODO is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@byliuyang Just added a TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@byliuyang sorry I still did not get why you think we should use QueryRow instead of Query, if we use QueryRow then it will cost us more connections with DB, we could definitely do performance test, but intuitively I feel it's slower

Copy link
Member

Choose a reason for hiding this comment

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

@dawntomm All queryRows in a single prepare statement share one connection.

if err != nil {
return urls, nil
}

defer rows.Close()
for rows.Next() {
url := entity.URL{}
err := rows.Scan(
&url.Alias,
&url.OriginalURL,
&url.ExpireAt,
&url.CreatedAt,
&url.UpdatedAt,
)
if err != nil {
return urls, err
}

url.CreatedAt = utc(url.CreatedAt)
url.UpdatedAt = utc(url.UpdatedAt)
url.ExpireAt = utc(url.ExpireAt)

urls = append(urls, url)
}

return urls, nil
}

// composeParamListString converts an slice to a parameters string with format: $1, $2, $3, ...
magicoder10 marked this conversation as resolved.
Show resolved Hide resolved
func (u URLSql) composeParamList(numParams int) string {
params := make([]string, 0, numParams)
for i := 0; i < numParams; i++ {
params = append(params, fmt.Sprintf("$%d", i+1))
}

parameterStr := strings.Join(params, ", ")
return parameterStr
}

// NewURLSql creates URLSql
func NewURLSql(db *sql.DB) *URLSql {
return &URLSql{
Expand Down
81 changes: 81 additions & 0 deletions backend/app/adapter/db/url_integration_test.go
View file
Open in desktop
Expand Up @@ -242,6 +242,87 @@ func TestURLSql_Create(t *testing.T) {
}
}

func TestURLSql_GetByAliases(t *testing.T) {
twoYearsAgo := mustParseTime(t, "2017-05-01T08:02:16-07:00")
now := mustParseTime(t, "2019-05-01T08:02:16-07:00")

testCases := []struct {
name string
tableRows []urlTableRow
aliases []string
hasErr bool
expectedURLs []entity.URL
}{
{
name: "alias not found",
tableRows: []urlTableRow{},
aliases: []string{"220uFicCJj"},
hasErr: false,
},
{
name: "found url",
tableRows: []urlTableRow{
{
alias: "220uFicCJj",
longLink: "http://www.google.com",
createdAt: &twoYearsAgo,
expireAt: &now,
updatedAt: &now,
},
{
alias: "yDOBcj5HIPbUAsw",
longLink: "http://www.facebook.com",
createdAt: &twoYearsAgo,
expireAt: &now,
updatedAt: &now,
},
},
aliases: []string{"220uFicCJj", "yDOBcj5HIPbUAsw"},
hasErr: false,
expectedURLs: []entity.URL{
entity.URL{
Alias: "220uFicCJj",
OriginalURL: "http://www.google.com",
CreatedAt: &twoYearsAgo,
ExpireAt: &now,
UpdatedAt: &now,
},
entity.URL{
Alias: "yDOBcj5HIPbUAsw",
OriginalURL: "http://www.facebook.com",
CreatedAt: &twoYearsAgo,
ExpireAt: &now,
UpdatedAt: &now,
},
},
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
mdtest.AccessTestDB(
dbConnector,
dbMigrationTool,
dbMigrationRoot,
dbConfig,
func(sqlDB *sql.DB) {
insertURLTableRows(t, sqlDB, testCase.tableRows)

urlRepo := db.NewURLSql(sqlDB)
urls, err := urlRepo.GetByAliases(testCase.aliases)

if testCase.hasErr {
mdtest.NotEqual(t, nil, err)
return
}
mdtest.Equal(t, nil, err)
mdtest.Equal(t, testCase.expectedURLs, urls)
},
)
})
}
}

func insertURLTableRows(t *testing.T, sqlDB *sql.DB, tableRows []urlTableRow) {
for _, tableRow := range tableRows {
_, err := sqlDB.Exec(
Expand Down
1 change: 1 addition & 0 deletions backend/app/usecase/repository/url.go
View file
Open in desktop
Expand Up @@ -7,4 +7,5 @@ type URL interface {
IsAliasExist(alias string) (bool, error)
GetByAlias(alias string) (entity.URL, error)
Create(url entity.URL) error
GetByAliases(aliases []string) ([]entity.URL, error)
}
15 changes: 15 additions & 0 deletions backend/app/usecase/repository/url_fake.go
View file
Open in desktop
Expand Up @@ -45,6 +45,21 @@ func (u URLFake) GetByAlias(alias string) (entity.URL, error) {
return url, nil
}

// GetByAliases finds all URL for a list of aliases
func (u URLFake) GetByAliases(aliases []string) ([]entity.URL, error) {
var urls []entity.URL

for _, alias := range aliases {
url, err := u.GetByAlias(alias)

if err != nil {
return urls, err
}
urls = append(urls, url)
}
return urls, nil
}

// NewURLFake creates in memory URL repository
func NewURLFake(urls map[string]entity.URL) URLFake {
return URLFake{
Expand Down