search

aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
774 stars 56 forks source link

Is it ok that `get_user!/1` is generated but only used by tests? #55

Closed zorn closed 4 years ago

zorn commented 4 years ago

One of the notes I took while learning this generator was the curious generation of get_user!/1 that is not used in the actual code but only the tests. This leads me to ask two questions:

  1. Should we be generating code in the public Accounts module if it is not currently or intended to be used by other controllers?
  2. If we generate this in the anticipation that a user will extend their project to say provide a users/:id/profile path, should they be using a bang ! function in said controller to get that user? When doing resource lookup on a path like this should we just assume a Phoenix project will catch a Ecto.NoResultsError error and show a 404 page? I am still kind of new to Phoenix and community norms but my initial reaction is that a custom module like Accounts should not throw Ecto errors.

Posting here to start a conversation. Thanks.

josevalim commented 4 years ago

Regarding tests, there are two camps here: some people believe it is fine to extend the domain function to add functionality related of the tests, since they consider testing to be a consumer of your code. And you generally want the tests to go through the domain, as much as possible, to ensure data consistency. While others believe this should not be done ever, it depends on which camp you sit. :)

However, in practice "Accounts.get_user!" is not used by controllers but it is used by other domains/contexts. This is exactly what is happening in the app I am working on and I extracted this functionality from. And yes, if get_user! errors, Ecto.NoResultsError is raised and it shows a 404 page. :)

my initial reaction is that a custom module like Accounts should not throw Ecto errors.

Consider Ecto to be the data representation that your domains chose to pass data around, between them and to controllers. That's why Ecto was split into Ecto and Ecto.SQL, exactly so you can have a complete set of data structures that works independently from databases. Your controllers can also fully depend on Ecto, without bringing any of the SQL functionality (if they happened to be on a separate app, for example). In this sense, returning Ecto data structures and errors is not different from choosing any other library, that may expose structs such as %URI{} and %Security.Encrypted{} that you would pass between contexts.

zorn commented 4 years ago

Thanks for the perspective. 👍