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

Conversation

dawntomm
Copy link
Contributor

@dawntomm dawntomm commented Mar 9, 2020

New Behavior

Description

Added GetByAliases function in db and repository.

User should be able to search the public and the private short links they created. In order to support that, we add RESTful API to search for all urls created by a given user. The first step is to add GetByAliases function to fetch URLs from database.

@dawntomm dawntomm self-assigned this Mar 9, 2020
@dawntomm dawntomm added this to In progress in Short 1.0 via automation Mar 9, 2020
@dawntomm dawntomm added this to the 03/07 - 03/13 milestone Mar 9, 2020
@dawntomm dawntomm linked an issue Mar 9, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #539 into master will increase coverage by 0.51%.
The diff coverage is 87.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #539      +/-   ##
=========================================
+ Coverage   78.39%   78.9%   +0.51%     
=========================================
  Files          35      35              
  Lines         847     896      +49     
=========================================
+ Hits          664     707      +43     
- Misses        153     156       +3     
- Partials       30      33       +3
Impacted Files Coverage Δ
backend/app/adapter/db/url.go 93.04% <87.75%> (-3.93%) ⬇️

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 8f8c1e8...480c5d1. Read the comment docs.

backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
Short 1.0 automation moved this from In progress to In Review Mar 9, 2020
@alldroll
Copy link
Contributor

alldroll commented Mar 9, 2020

Thank you for your PR!. Please address my comments :)

backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url_integration_test.go Outdated Show resolved Hide resolved
backend/app/adapter/db/.env Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
}
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.

backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
backend/app/adapter/db/url.go Outdated Show resolved Hide resolved
@magicoder10
Copy link
Member

magicoder10 commented Mar 12, 2020

@alldroll Tom addressed all your comments. I'll merge now to unblock him so that he can work on the next component.

@magicoder10 magicoder10 merged commit 314ce78 into short-d:master Mar 12, 2020
Short 1.0 automation moved this from In Review to Done Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Short 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

Create RESTful search API in Go
4 participants