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

PB-1045 Add user model and GET endpoints #30

Merged
merged 24 commits into from
Oct 21, 2024

Conversation

asteiner-swisstopo
Copy link
Contributor

@asteiner-swisstopo asteiner-swisstopo commented Oct 10, 2024

This adds the GET endpoints for users in service-control. The POST endpoint is done in a subsequent PR in order to unblock the works on adding the users to Cognito (see PB-1044).

Summary of changes:

  1. New app "access" as defined in the API specs.
  2. New Django model User with
     username: CharField
     first_name: CharField
     last_name: CharField
     email: EmailField
     provider: ForeignKey
    ℹ️ The fields created and updated that are listed in the API specs are left out for the moment.
    ℹ️ We defined our own user model instead of taking the one from django.contrib.auth. This is to keep a minimal model and avoid confusion that Django authentication has anything to say here.
  3. New endpoints:
    1. users/{user_id}: Returns a specific user.
      ℹ️ For the response schema, I considered setting type EmailStr for field UserSchema.email. I didn't do so because that would introduce an additional dependency: email-validator. I thought that the validation in the Django model is enough but happy to adjust if the response should also be validated.
      ℹ️ Also for the response schema UserSchema, I considered inheriting from ninja.ModelSchema. This would have allowed me to avoid repeating all the fields and using UserSchema(model).from_orm() to convert the model to the schema. I couldn't manage to get this working because Django somehow creates implicit fields like provider_id_id (note the double _id appendix). Instead, I added a function user_to_response similar to the other models. Please let me know if you see a way how to define the model/schema fields such that we can make use of ninja.ModelSchema.
    2. users: Returns all users.

Example response for users/123:

{
    "username": "dude",
    "first_name": "Jeffrey",
    "last_name": "Lebowski",
    "email": "[email protected]",
    "provider_id": 42,
}

Example response for users:

{
    "items": [
        {
            "username": "dude",
            "first_name": "Jeffrey",
            "last_name": "Lebowski",
            "email": "[email protected]",
            "provider_id": 42,
        },
        {
            "username": "veteran",
            "first_name": "Walter",
            "last_name": "Sobchak",
            "email": "[email protected]",
            "provider_id": 42,
        },
    ]
}

Copy link
Contributor

@schtibe schtibe left a comment

Choose a reason for hiding this comment

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

Nice, just little remarks.
Now go bowling!

app/config/api.py Show resolved Hide resolved
app/access/api.py Outdated Show resolved Hide resolved
app/access/tests/test_api.py Show resolved Hide resolved
Copy link
Contributor

@msom msom left a comment

Choose a reason for hiding this comment

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

Looks good.

Regarding django vs. ninja validation: Have you tried what happens if in invalid email is provided (might be worth a test)? My guess is that relying on the django validator will ultimately result in an aborted transaction, where using the ninja validator will probably not. If true, using the ninja validator might be a bit cleaner.

@asteiner-swisstopo
Copy link
Contributor Author

asteiner-swisstopo commented Oct 14, 2024

Looks good.

Regarding django vs. ninja validation: Have you tried what happens if in invalid email is provided (might be worth a test)? My guess is that relying on the django validator will ultimately result in an aborted transaction, where using the ninja validator will probably not. If true, using the ninja validator might be a bit cleaner.

As discussed in chat, Django's EmailField only affects form validation. Not model validation. So no exception is raised when saving a User with an invalid email. I added a corresponding test to demonstrate that.

For the moment, we agreed on taking the extra dependency and add email-validator such that we can use pydantics EmailStr. To be discussed in the team whether we want to validate only input/output at the endpoints or also when storing data to the DB.

@asteiner-swisstopo asteiner-swisstopo self-assigned this Oct 15, 2024
@asteiner-swisstopo
Copy link
Contributor Author

Hi @schtibe and @msom,

I have adapted the following after our discussion yesterday:

  1. Add field User.username as primary key.
  2. Leave User.first_name / User.last_name as it is.
  3. Overwrite User.clean() and User.save() such that the email es validated every time we try to write to the database.
  4. Remove the type checks in UserSchema.email and User.email. Both are not necessary anymore with the validation in User.clean(). This also allows us to avoid the additional dependency email-validator.

Would you mind having another look please? 😇

app/access/migrations/0001_initial.py Outdated Show resolved Hide resolved
app/access/models.py Outdated Show resolved Hide resolved
This is going to accommodate users and policies.
This probably only showed now  that there are other tests adding more Providers in parallel.
This makes it work the same as for end points in apps distributions ("/attributions", "/datasets") and access ("/users"). Also, it's better visible in the tests what is the full end path to the end point.
Of interest here because it demonstrates that the email is only validated in forms, not when writing to the DB. We check that currently only on end point level using Ninja schemas.
This introduces a new dependency: email-validator.
So the data is always validated no matter where it comes from (REST API, form, admin, CLI, ...).
This avoid having an extra function Model.clean() and the constraint is closer to the field definition. As we instruct Model.save() to call Model.full_clean(), this validator is going to be executed every time we save the model.
@asteiner-swisstopo asteiner-swisstopo force-pushed the feat-PB-1045-add-user-model-and-endpoint branch from d87180a to 1aee531 Compare October 21, 2024 09:54
This fixes unit test CheckerUrlTestCase.test_checker_url that reported error "ninja.errors.ConfigError: Router@'' has already been attached to API NinjaAPI:1.0.0" when run in parallel with other tests.

A reason for the error might have been that this test introduced a mix of the django test client and the ninja test client. The ninja test client test skips the middleware/url-resolver layer while the django test client does not. So somehow Ninja's router registration was done twice.

Just using the ninja test client wasn't enough then because it didn't know the "/checker" endpoint, which was only defined in the "urls" module. So the "/checker" endpoint is now also handled by its own NinjaAPI object.

In the course of this, the definition of the "/checker" endpoint was moved back to the "api" module where API endpoints are usually defined.
This fixes unit test CheckerUrlTestCase.test_checker_url that reported error "ninja.errors.ConfigError: Router@'' has already been attached to API NinjaAPI:1.0.0" when run in parallel with other tests.

A reason for the error might have been that this test introduced a mix of the django test client and the ninja test client. The ninja test client test skips the middleware/url-resolver layer while the django test client does not. So somehow Ninja's router registration was done twice.

Just using the ninja test client wasn't enough then because it didn't know the "/checker" endpoint, which was only defined in the "urls" module. So the "/checker" endpoint is now also handled by its own NinjaAPI object.

In the course of this, the definition of the "/checker" endpoint was moved back to the "api" module where API endpoints are usually defined.
This addresses warning "(urls.W005) URL namespace 'api-1.0.0' isn't unique. You may not be able to reverse all URLs in this namespace".
@asteiner-swisstopo asteiner-swisstopo merged commit 1915ec8 into develop Oct 21, 2024
3 checks passed
@asteiner-swisstopo asteiner-swisstopo deleted the feat-PB-1045-add-user-model-and-endpoint branch October 21, 2024 13:56
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.

6 participants