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

導入router #124

Conversation

Julian-Chu
Copy link
Contributor

討論用的PR

👏 解決掉的 issue / Resolved Issues

📝 相關的 issue / Related Issues

⛏ 變更內容 / Details of Changes

  • 引入 第三方router (目前用gorilla/mux, 可替換)
  • 對router封裝
  • 改寫/v1/boards做為討論範例

採用第三方router的好處,可以大幅減少route的實作邏輯,route的管理也直觀,有利於下一步導入middleware,加上router很輕量,沒有依賴單一第三方套件的風險

@Julian-Chu
Copy link
Contributor Author

Copy link

@asymptoter asymptoter left a comment

Choose a reason for hiding this comment

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

註解掉的地方可能要說明一下原因

// case http.MethodGet:
// delivery.getBoards(w, r)
// }
//}

Choose a reason for hiding this comment

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

這邊 comment 掉是為什麼呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

單純只是改寫v1/boards 後會移除的部分, 因為是討論用途, 所以先留著比對

// w.WriteHeader(http.StatusNotFound)
//
// delivery.logger.Noticef("board id: %v not exist but be queried, info: %v err: %v", boardId, item, err)
//}

Choose a reason for hiding this comment

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

同上

// t.Errorf("handler returned wrong status code: got %v want %v",
// status, http.StatusBadRequest)
// }
//}

Choose a reason for hiding this comment

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

同上

func (delivery httpDelivery) Post(path string, handlerFunc http.HandlerFunc) {
delivery.Router.HandleFunc(path, handlerFunc).Methods(http.MethodPost)
}
func (delivery httpDelivery) Get(path string, handlerFunc http.HandlerFunc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

這幾個 func (delivery httpDelivery) 會不會改成 func (delivery *httpDelivery) 比較好呢
主要有兩點
因為其他 httpDelivery methods 都是 pointer type,可以統一一下 XD
還有註冊 delivery.Router.HandleFunc 是去改 httpDelivery 裡面的 Router 裡面的 state,看起來比較適合用 pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

阿 謝了 沒注意到 後面會修正

@Markogoodman
Copy link
Contributor

大概看了一下覺得不錯 XD
目前用的功能都方便也不會太多依賴
之後如果發現gorilla mux有什麼功能好用,想用又怕有依賴的話再拿出來討論也可以

@Julian-Chu
Copy link
Contributor Author

  • 精華區的目錄可以有多層, 直接從 route parameters 取會有困難, 暫時維持原樣
  • 還有 class 的部分未完成,
  • TestGetUserInformation 更改成直接對 httpdelivery 測試, 不用額外註冊route (其他的 httpdelivery test 也建議改動)

@Julian-Chu
Copy link
Contributor Author

遇到一個問題, 中間剛好 merge linter 進 development, merge conflict 有點多, 直接處理怕會遺漏, 如果覺得引入router ok的話, 我打算關掉這個 PR, 從最新的 development做

@PichuChen
Copy link
Member

遇到一個問題, 中間剛好 merge linter 進 development, merge conflict 有點多, 直接處理怕會遺漏, 如果覺得引入router ok的話, 我打算關掉這個 PR, 從最新的 development做

我覺得可以關掉重做,不過在這之前因為引入 router 的意見還是比較分歧,所以不知道能不能像是 go-bbs cache 資料庫的那種形式,稍微提出一下 benchmark 結果以及是否引入的差異這樣?

@Julian-Chu
Copy link
Contributor Author

Julian-Chu commented Apr 5, 2021

遇到一個問題, 中間剛好 merge linter 進 development, merge conflict 有點多, 直接處理怕會遺漏, 如果覺得引入router ok的話, 我打算關掉這個 PR, 從最新的 development做

我覺得可以關掉重做,不過在這之前因為引入 router 的意見還是比較分歧,所以不知道能不能像是 go-bbs cache 資料庫的那種形式,稍微提出一下 benchmark 結果以及是否引入的差異這樣?

我只做了 DefaultServeMux 跟 GorillaMux
BenchmarkGetBoardArticlesBadRequest_GorillaMux-7 4688 263512 ns/op
BenchmarkGetBoardArticlesBadRequest_DefaultMux-7 21082 57909 ns/op

針對其他的router 可以參考 go-http-routing-benchmark]
GorillaMux是其中比較慢的router, 換成其他的可以有 4 - 10倍的提升, 就可以跟原生的效能打平

另外以目前的 route 不會遇到 httprouter wildcard的問題

@Julian-Chu
Copy link
Contributor Author

另外差異較大的是在route的管理方面,
採用 router 的話, 已經實作的路徑一目瞭然, 同時不需要額外實作 route

func (delivery *httpDelivery) buildRoute() {
	// TODO: Check IP Flowspeed
	delivery.Post("/v1/token", delivery.postToken)
	// TODO: Check IP Flowspeed
	delivery.Get("/v1/boards", delivery.getBoardList)
	delivery.Get("/v1/boards/{boardID}/information", delivery.getBoardInformation)
	delivery.Get("/v1/boards/{boardID}/articles", delivery.getBoardArticles)
	delivery.Get("/v1/boards/{boardID}/articles/{filename}", delivery.getBoardArticlesFile)
	delivery.Get("/v1/boards/{boardID}/treasures/", delivery.getBoardTreasures)
	// TODO: Check IP Flowspeed
	delivery.Get("/v1/popular-boards", delivery.getPopularBoardList)
	delivery.Get("/v1/popular-articles", delivery.getPopularArticles)
	// TODO: Check IP Flowspeed
	delivery.Get("/v1/classes", delivery.getClassesWithoutClassId)
	delivery.Get("/v1/classes/{classID}", delivery.getClassesList)
	// TODO: Check IP Flowspeed
	delivery.Get("/v1/users/{userID}/information", delivery.getUserInformation)
	delivery.Get("/v1/users/{userID}/favorites", delivery.getUserFavorites)
}

以原本的 boards route 為例, 新功能就會需要更動 delivery.routeBoards, 增加分支邏輯
同時管理上需要查看 routeBoards 的邏輯 才能確認有那些 subroute 實作了

func (delivery *Delivery) buildRoute(mux *http.ServeMux) {
	mux.HandleFunc("/v1/token", delivery.routeToken)
	mux.HandleFunc("/v1/boards", delivery.routeBoards)
	mux.HandleFunc("/v1/boards/", delivery.routeBoards)
	mux.HandleFunc("/v1/popular-boards", delivery.routePopularBoards)
	mux.HandleFunc("/v1/popular-articles", delivery.routePopularArticles)
	mux.HandleFunc("/v1/classes/", delivery.routeClasses)
	mux.HandleFunc("/v1/users/", delivery.routeUsers)
}

引入方面應該是沒有問題呢, 之前的討論點主要是在有沒有要採用 gin 或是 httprouter

@Julian-Chu
Copy link
Contributor Author

另外像是以下的 parse方法都可以在引入 router 後拿掉,

// getBoards is the handler for `/v1/boards` with GET method
func (delivery *Delivery) getBoards(w http.ResponseWriter, r *http.Request) {
	delivery.logger.Debugf("getBoards: %v", r)
	boardID, item, filename, err := delivery.parseBoardPath(r.URL.Path)
	if boardID == "" {
		delivery.getBoardList(w, r)
		return
	}
	// get single board
	if item == "information" {
		delivery.getBoardInformation(w, r, boardID)
		return
	} else if item == "articles" {
		if filename == "" {
			delivery.getBoardArticles(w, r, boardID)
		} else {
			delivery.getBoardArticlesFile(w, r, boardID, filename)
		}
		return
	} else if item == "treasures" {
		delivery.getBoardTreasures(w, r, boardID)
		return
	}

	// 404
	w.WriteHeader(http.StatusNotFound)

	delivery.logger.Noticef("board id: %v not exist but be queried, info: %v err: %v", boardID, item, err)
}

@Julian-Chu
Copy link
Contributor Author

原本 subroute 的參數會由 route 傳入, 以 boardID 為例

func (delivery *Delivery) getBoardInformation(w http.ResponseWriter, r *http.Request, boardID string) {
	delivery.logger.Debugf("getBoardInformation: %v", r)
	token := delivery.getTokenFromRequest(r)

改用 router 後, 會直接用 Params 方法取值, 所有方法的簽章都改為符合 httpHandleFunc

func (delivery *httpDelivery) getBoardInformation(w http.ResponseWriter, r *http.Request) {
	params := delivery.Params(r)
	boardID := params["boardID"]
	delivery.logger.Debugf("getBoardInformation: %v", r)
	token := delivery.getTokenFromRequest(r)

@Julian-Chu
Copy link
Contributor Author

問看看有沒有人想要接手做, 我可能要五月初才有時間orz

@Julian-Chu Julian-Chu closed this Apr 15, 2021
@PichuChen
Copy link
Member

不好意思我剛剛才看到有回應benchmark了, benchmark 的部分有放在哪個branch嗎?

@Julian-Chu
Copy link
Contributor Author

@PichuChen
我切換 branch 下去做的, 沒有 commit 進去, 這幾天我找時間寫一個新的

@PichuChen
Copy link
Member

PichuChen commented Apr 19, 2021 via email

@Julian-Chu Julian-Chu mentioned this pull request May 10, 2021
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.

4 participants