This repository was archived by the owner on Dec 20, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…c tests for login and authorize
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a very large refactor. Nearly every file has been changed. The primary reason for this was that it was starting to become harder to separate the application logic from the authentication logic. So, I separated out every part of the code that is necessary for the "authorization server (as per the OAuth standard" into a new module, called
auth
.To still allow database access from there, the database primitives have been separated out into a more abstracted form,
store
. Theauth
module directly uses these primitives when it does not depend on any particular schema (such as when using the KV). When it does require a schema, it uses a set of Protocol classes which define only the type information. These are not implemented inside theauth
module as it would have too many dependencies on the application specifics. The goal is that theauth
module can in the future be separated and used with a completely different web framework (so it makes no assumptions about e.g. FastAPI). The consuming package (in this case our own application, theapiserver
) then implements these schema-dependent functions (primarily the refresh token storage, as well as specifics about users and user data). Theauth
package still imposes some pretty strict limitations on the design of any consuming package, but the end goal is not a perfect API, mostly to more cleanly separate our own logic.The code wasn't just copied to the new module, it was neatly separated into a much larger number of files, to make sure each individual file stays small and maintainable. Functions should mostly not do too many things at once. On the application side, user and userdata have been separated out a bit.
A technique used to make things more testable is what I've termed "context" (when used inside
auth
) and "frame" (when used inside the application). The different names is just to easily distinguish them, the concept is the same. I like having my functions defined at the top level, so I used decorators to still register them inside a Protocol class that is the one that is actually called. This is only for "data" functions, so functions whose primary role is to interface with the database. These Protocol classes can be subclassed in tests quite easily with mock implementations. Previously, we used the builtinunittest.mock
library, but you had to use strings to define the file locations (so when you refactored these didn't update) and they relied quite heavily on at what moment files are imported, which is just too much to reason about. This way you guarantee the code is properly replaced by the mock implementation. I could have just created normal classes and defined the functions there, at some point I might revert back to that if this is too complex, but I like the final structure.I will merge this into
klassement_db
, which will then get the final features to update the klassement.