Skip to content
This repository was archived by the owner on Oct 8, 2022. It is now read-only.

Conversation

@moreirathomas
Copy link
Member

@moreirathomas moreirathomas commented Feb 22, 2022

Description

Architectural changes and sync with benchttp/runner regarding data structure and endpoints.

Changes

  • changes packages structure: server (previous http) is at the root, benchttp entities are inside benchttp
  • swap Benchmark and Report structs
  • rework HTTP response writing for JSON and errors (not better or worst, mainly experiments, subject to discussion)
  • /v1 subrouter

Notes

Base automatically changed from feat/worker-call to main February 27, 2022 17:08
- http becomes the root as server
- benchttp structs definition moves to benchttp package
- httplog is a helper module at the root
- swap Benchmark field with Report
- update firestore service
- update server injectees
@moreirathomas moreirathomas marked this pull request as ready for review February 27, 2022 20:06
@moreirathomas moreirathomas changed the title dev: archi feat: sync with benchttp/runner Feb 27, 2022
@moreirathomas moreirathomas added enhancement New feature or request must have labels Feb 27, 2022
Copy link
Member

@GregoryAlbouy GregoryAlbouy left a comment

Choose a reason for hiding this comment

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

Even though I'm not a fan of having source files at the root mixed with config files, it works really well here with the name of the module (e.g. server.New).
I also really like the domain package benchttp, I'm wondering whether we should consider porting it to the runner (might not work as well as there's much more going on with the equivalent entities).

Comment on lines 37 to 41
// CloseClient calls the Close method of the internal client
// of the ReportService.
func (s ReportService) CloseClient() {
s.client.Close()
}
Copy link
Member

Choose a reason for hiding this comment

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

[question] This is not used, what should we do about it?

[nitpick] I'd keep the implementation details to the implementation itself, considering the underlying client is private.

Suggested change
// CloseClient calls the Close method of the internal client
// of the ReportService.
func (s ReportService) CloseClient() {
s.client.Close()
}
// Close closes the connection to Firestore client.
func (s ReportService) Close() error {
return s.client.Close()
}

Copy link
Member

@GregoryAlbouy GregoryAlbouy left a comment

Choose a reason for hiding this comment

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

🚀

My only remaining concerns are:

  • the 405 returned in certain cases of invalid id: not ideal but secondary and can be improved later
  • the endpoint path that I believe would be more conventional in format /reports/{id}, but the query param works well too so let's let the author choose 🤷‍♂️

Copy link
Member

@GregoryAlbouy GregoryAlbouy left a comment

Choose a reason for hiding this comment

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

😍

@moreirathomas
Copy link
Member Author

the 405 returned in certain cases of invalid id: not ideal but secondary and can be improved later

Only thing remaining. What happens:

  • /reports accepts POST requests to write a new report
  • /reports/{id} accepts GET requests to read an existing report

Calling /reports with a GET request is by definition a method not allowed.

@GregoryAlbouy
Copy link
Member

GregoryAlbouy commented Mar 1, 2022

Calling /reports with a GET request is by definition a method not allowed.

Yes, now it makes perfect sense because the two endpoints do not share the same path anymore.
What was happening is that because the ID didn't match the defined GET /report route, the router was falling back to the POST /report hence the Method not allowed, I guess.

@moreirathomas moreirathomas merged commit 7991464 into main Mar 1, 2022
@moreirathomas moreirathomas deleted the dev/archi branch March 1, 2022 20:36
@moreirathomas moreirathomas added this to the Release 1.0.0 milestone Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request must have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants