Lasagna-Love-Portal / bechamel-api

Bechamel: a REST API for Lasagna Love request, requester and volunteer management
Eclipse Public License 2.0
5 stars 4 forks source link

Initial pass at a beginning sketch for API server. #1

Closed mattkuznicki-ll closed 1 year ago

mattkuznicki-ll commented 1 year ago

Simple implementations for login (JWT authentication token acquisition), creating new user profiles, and obtaining profile for the current authenticated user + for a profile by ID.

This is very much a starting point and there's a lot missing from here. In no specific order, known issues include:

The intent of this addition is to give a starting point for project participants to be able to start working on the API and to give a concrete forum for prototyping potential API additions.

This is the author's first Go project - pointers and tips on style, conventions, ways to improve code or anything else are welcomed.

mattkuznicki-ll commented 1 year ago

LGTM!

Thanks @pid1 - let's please leave this open at least a day or two to give others a chance to review before merging.

mattkuznicki-ll commented 1 year ago

While this is certainly early days, I'd like to see us start with a more intentional and structured approach. I've also got some questions around the general approach here.

  1. I'm curious about the choice here between gin, chi, and the built-in net/http. I've generally preferred chi, but I'm pretty flexible on this. Mostly want to make sure we're picking one on purpose.
  2. I usually keep main.gos in cmd/[usecase]/: I've often found reasons to want to compile and run the logic in different ways, and this allows for pretty easy extension
  3. I like to keep files specific to their use, especially main.go. For an HTTP service, that usually means that main.go only contains: call to config package, dependency initializer, router generation (w/ dependency injection), and server start (w/ error capture, and ideally SIGINT capture)
  4. I'm not a fan of an internal package. I usually separate it out into specific domains. We can probably nest them inside internal/, but I don't like a general catch-all package.
  5. I want to keep the data model / business logic fully separated from the HTTP layer. The http layer should import the data model layer.

I think the closest I've got to an example would be: https://gitlab.com/mk5r/rialto-exercises , but that's not fully complete on those points: it was a super quick-and-dirty coding challenge for a job app, so it's missing some things (such as the logger isn't being dependancy-injected at handler generation). It's probably also correct to move person/ into internal/ there.

Thanks for the feedback @mkantzer! To your points:

  1. I went with Gin for this based on example availability and perceived activity vs. chi, either seems a perfectly adequate choice here IMO. I'll suggest Gin's perfectly good until we have a problem with it but I'd happily look at a PR that switched it out for chi or went for net/http usage.
  2. Acknowledged, suggest we split off when we have better case for doing so.
  3. Agreed. I've separated out some in 4f93e66.
  4. There seems not to be any real consensus on how to handle this sort of thing. I'm open to nesting inside of an internal package if needed - maybe let's see how complicated things get later? A lot of functionality is going to be housed in microservices Bechamel uses rather than in Bechamel itself.
  5. Not sure I understand. With the changes in 4f93e66 is there more separation you'd like to see?
iamthebot commented 1 year ago

While this is certainly early days, I'd like to see us start with a more intentional and structured approach. I've also got some questions around the general approach here.

  1. I'm curious about the choice here between gin, chi, and the built-in net/http. I've generally preferred chi, but I'm pretty flexible on this. Mostly want to make sure we're picking one on purpose.
  2. I usually keep main.gos in cmd/[usecase]/: I've often found reasons to want to compile and run the logic in different ways, and this allows for pretty easy extension
  3. I like to keep files specific to their use, especially main.go. For an HTTP service, that usually means that main.go only contains: call to config package, dependency initializer, router generation (w/ dependency injection), and server start (w/ error capture, and ideally SIGINT capture)
  4. I'm not a fan of an internal package. I usually separate it out into specific domains. We can probably nest them inside internal/, but I don't like a general catch-all package.
  5. I want to keep the data model / business logic fully separated from the HTTP layer. The http layer should import the data model layer.

I think the closest I've got to an example would be: https://gitlab.com/mk5r/rialto-exercises , but that's not fully complete on those points: it was a super quick-and-dirty coding challenge for a job app, so it's missing some things (such as the logger isn't being dependancy-injected at handler generation). It's probably also correct to move person/ into internal/ there.

Re: gin vs. net/http vs. chi at this point, there's not too compelling a reason to stick purely with net/http nowadays (frankly, even if you want a lower level interface you should be using fasthttp anyways) and there's more an ecosystem around gin now which edges it out IMO. Both are fine and there are, IMO, more important technical decisions than this for this to be a blocking issue. Keep in mind, communication between services will be GRPC, this is only for the main REST API.

I like to keep files specific to their use, especially main.go. For an HTTP service, that usually means that main.go only contains: call to config package, dependency initializer, router generation (w/ dependency injection), and server start (w/ error capture, and ideally SIGINT capture)

It's no problem at all moving the code around as more functionality is added. I generally do agree with the sentiment here but it's not really a blocker given the small amount of code.

I usually keep main.gos in cmd/[usecase]/: I've often found reasons to want to compile and run the logic in different ways, and this allows for pretty easy extension

This is also a great way to end up with subtle bugs if your interfaces have slightly changed in your various main.go's. This is not a practice I would generally endorse vs. a single CLI that can be run in different modes (which is a more industry standard way of handling eg; various deployment modes using a single binary).

I'm not a fan of an internal package. I usually separate it out into specific domains. We can probably nest them inside internal/, but I don't like a general catch-all package.

I agree. Ultimately the arrangement depends on what exactly you want importable as a library elsewhere. My thought model here is we won't really be importing much from this repo directly since what talks to this will be frontends including a mobile app down the line. So let's just keep everything in one package, ditch the internal package and we can nest if needed as this grows. Not a big deal b/c you won't break downstream imports.

mkantzer commented 1 year ago

@mattkuznicki-ll Not quite, although in review it's likely I'm applying things early here.

I usually end up keeping 2 separate struct defs for API'd objects:

On of the functions of the handlers is to convert between the two.

The basic idea is to be able to change either implementation without needing to change the entire application, and to keep each fully testable in isolation.

Guiding principal: your API should not be directly tied to ex: a database schema. They're related, but should not be required to be the same.

mattkuznicki-ll commented 1 year ago

@mkantzer totally agree we don't want a schema that's directly tied to the database, nor to end up exposing database structure to callers directly. The database access is intended to end up in a separate microservice used by the Bechamel API, and I believe you'll see more separation once that gets closer to an implementation. Including the ability to test components individually. For the near term, the primary objective is to get something we can flesh out an API with so those working on the frontend have something to work with & code to. So it may be a bit before the backend becomes closer to its componentized system that we intend it to become.

mattkuznicki-ll commented 1 year ago

Seeing 3 approvals, am merging. We can refine in issues and later PRs.

patmejia commented 1 year ago

@mattkuznicki-ll, outstanding API sketch and authentication implementation. well done & thank you!

Roadmap

tech talk to-do:

✎ unit tests: Discussion on implementation ✎ mock data: Separating mock/stub data ✎ password hashing: Future plan ✎ frameworks: Gin, Chi, net/http consideration ✎ file structure: Organizing files and main.go ✎ data models: Separation of data models from HTTP layer (i.e encapsulating database access into a separate microservice)