Add support for PATCH to partially replace an entity

michaelarnauts commented 5 months ago

~This PR adds support for PUT requests to fully replace an entity in one transaction. Note that you'll need to supply the full object. Non-supplied checks or linked users for example will be removed.~

This PR adds support for PATCH requests to partially replace an entity in one transaction. You'll need to pass the properties you want to replace. If you want to replace the full object, specify all properties.

See https://github.com/angely-dev/freeradius-api/issues/5

michaelarnauts commented 5 months ago

I'll update this PR to add support for PATCH requests also, since I can refactor this a bit.

michaelarnauts commented 5 months ago

I've added support for PATCH.

I could refactor to create a BaseUser and have a UserCreate and UserUpdate inherit from this BaseUser. The username could then be in the UserCreate class (since UpdateUser doesn't need to username in the body), and the UserUpdate would just extend from BaseUser with only an updated model_config.

I think this might make the code more complicated though, so I leave it up to you to indicate if you want to have some changes.

angely-dev commented 5 months ago

Thanks for the work and your interest in the project. I do appreciate!

I reviewed the commit for the PUT and couldn't have done it better, except for the status code that should be 200 instead of 201. Theoretically, it may also create the object if it does not exist already (and in this case, a 201 is returned, yes) as per RFC 9110.

What you implemented in the PATCH is precisely the JSON Merge Patch strategy from RFC 7386 (e.g., if an attribute is not given, we don't modify it). For reference, FastAPI did document it in Partial updates with PATCH and it makes use of Pydantic exclude_unset feature.

I'm okay with this implementation and I agree: at this point, we might as well refactor the classes with inheritance. For reference again, FastAPI did suggest a similar pattern, especially about the naming convention (UserBase, UserCreate and so UserUpdate). Feel free to refactor this if you wish. Otherwise, I'll probably do it myself later.

I can either accept the PR as is or wait for the refactor.

michaelarnauts commented 5 months ago

Thanks for your review and comment!

I reviewed the commit for the PUT and couldn't have done it better, except for the status code that should be 200 instead of 201. Theoretically, it may also create the object if it does not exist already (and in this case, a 201 is returned, yes) as per RFC 9110.

Note that I removed the PUT requests, since the same could be achieved by using the PATCH requests and just passing all the attributes with [].

I think for this merge request, the PATCH implementation is fine and the refactoring can be done later if needed. I don't think the PUT request is needed.

michaelarnauts commented 5 months ago

I'll update the PATCH implementations so it returns a 200 instead of a 201.

angely-dev commented 5 months ago

So I ran your PR in my local environment and all seems good.

Would you mind, however, add a bit more unit tests to get 100% code coverage of pyfreeadius as it is the case at the moment?

$ pip install coverage
$ coverage run -m unittest test.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.126s

OK

$ coverage report
Name              Stmts   Miss  Cover
-------------------------------------
database.py           5      0   100%
pyfreeradius.py     333     16    95%
test.py             102      0   100%
-------------------------------------
TOTAL               440     16    96%

You can see details and missing lines using the HTML report:

$ coverage html
Wrote HTML report to htmlcov/index.html

Also, could you sync your fork to get the last commit that removed the extra commit from NasRepository.add?

Thanks!

angely-dev commented 5 months ago

I was trying to add the missing unit tests and got some failures. Here are the problems I observed:

patch_user endpoints does not check groups (if given) existence (like this)—so we can add a non-existing group to a user
patch_group endpoints does not check users (if given) existence (like this)—so we can add a non-existing user to a group
We cannot update just the group users, e.g., GroupUpdate('my-existing-group', users=[gu, gu2]) will trigger a ValidationError because of these lines

So the PR is actually missing some semantic checks or logic.

michaelarnauts commented 5 months ago

My bad, I forgot to add those. I think I addressed these changes in my last commit.

angely-dev commented 5 months ago

Thanks for the changes.

One last concern: the patch_group endpoint now allows to remove all group attributes (checks and replies). And this I explicitly forbid during the conceptual stage. It will also trigger a ValidationError when returning the group because of these lines.

Example:

{
    "checks": [],
    "replies": [],
    "users": [{"username": "a", "priority": 1}, {"username":"b", "priority": 1}]
}

So the patch_group endpoint needs extra logic to compare the given attributes with the current ones.

Conceptually, having a group without any attributes (while permitted by the FreeRADIUS data model) is strange from the REST API point of view. It would mean that a user can belong to a non-existing group. The group will start to exist because a user has been bound to him, and will cease to exist once its last user has been deleted.

michaelarnauts commented 5 months ago

Hmm, I do have the use-case for a group without any attributes or checks. This is to make it easier later on to just add attribute to that group without having to link all users to that group. I prefer to link the users to their group during creation instead of later on when a parameter is added, since this information isn't easily available at that point.

angely-dev commented 5 months ago

So this is more of a conceptual change I prefer not to accept. I get your point, but this API is mainly made for automation purposes where users get created and updated (deleted and re-created) using a Source of Truth. It could be the same for groups (e.g., L3VPN groups shared among users of the same L3VPN). I don't see a practical need where an automatized tool will first create an empty group with users and then add the attributes later.

At this point, either we add more logic to prevent empty groups or we implement attributes overwriting as I suggested in https://github.com/angely-dev/freeradius-api/issues/5#issuecomment-2143377592.

You may disagree on the conceptual approach and I'd understand if you prefer to work on a separate fork with that enhancement.

michaelarnauts commented 5 months ago

I'll try to describe my use-case, and leave it up to you what you want to do with it :)

We will be using the API to push Radius users that is used for PPPoE sessions. We use a radius group to combine users that belong to the same Organisation. They can have multiple users if they have multiple connections on multiple locations.

Usually, we specify on a group level if the user needs to be terminated in a specific VRF but we can also provide different DNS servers here for example.

On occasion, these settings can change, or some other group level setting needs to be modified. (This is why I want to be able to PATCH them instead of deleting and recreating).

Now, while writing this, I noticed that we always have at least one attribute in the group (to specify the VRF) so the restriction on having at least one attribute might be okay for us.

angely-dev commented 5 months ago

Yes, the use case you described is a typical scenario for VRF-based users belonging to the same L3VPN and RADIUS groups are well suited for this. I'm okay with the PR but the extra logic in patch_group endpoint to ensure groups always have at least one attribute is needed. I could add it myself after accepting the PR, or you could implement it if you are willing to.

michaelarnauts commented 5 months ago

I can look at this next week :)

michaelarnauts commented 5 months ago

@angely-dev I've added the check to the patch_group(). I still have some doubts about this, since we need to do 3 extra queries now when updating a group, but this does allow for the business rules to be consistent, so I suppose it's worth it :)

I also noticed that I had to check for the checks, replies or groups to be actually None instead of falsy, to know the difference between an update with no values ([]), or not updating something.

I wanted to add a test for this, but this would require a rewrite of the tests, since they now test the repository itself, instead of the api methods where the additional business logic resides.

angely-dev commented 4 months ago

I reran the PR in my local environment and got errors:

When patching an object, lines 118, 131 and 148 throw an AttributeError since the name is not part the Update models, e.g.:

    raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}')
AttributeError: 'GroupUpdate' object has no attribute 'groupname'

I fixed it and got another error after patching my group which only has reply attributes:

fastapi.exceptions.ResponseValidationError: 3 validation errors:
  {'type': 'missing', 'loc': ('response', 'groupname'), 'msg': 'Field required', 'input': GroupUpdate(checks=None, replies=[AttributeOpValue(attribute='Filter-Id', op=':=', value='10m')], users=None)}
  {'type': 'list_type', 'loc': ('response', 'checks'), 'msg': 'Input should be a valid list', 'input': None}
  {'type': 'list_type', 'loc': ('response', 'users'), 'msg': 'Input should be a valid list', 'input': None}

That is because there is a mismatch between the response_model which is set to Group and the object you returning which is a GroupUpdate. The same applies for User and Nas. And also in the current code, I work with empty list rather than None.

Unit tests still not cover all the pyfreeradius module:

$ coverage run -m unittest test.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.143s

OK

$ coverage report
Name              Stmts   Miss  Cover
-------------------------------------
database.py           5      0   100%
pyfreeradius.py     333     16    95%
test.py             102      0   100%
-------------------------------------
TOTAL               440     16    96%

Note that I am working on the unit tests of the API itself as per Testing.

michaelarnauts commented 4 months ago

I reran the PR in my local environment and got errors:

* When patching an object, lines [118](https://github.com/michaelarnauts/freeradius-api/blob/allow-updates/src/api.py#L118), [131](https://github.com/michaelarnauts/freeradius-api/blob/allow-updates/src/api.py#L131) and [148](https://github.com/michaelarnauts/freeradius-api/blob/allow-updates/src/api.py#L148) throw an `AttributeError` since the `name` is not part the `Update` models, e.g.:

    raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}')
AttributeError: 'GroupUpdate' object has no attribute 'groupname'

* I fixed it and got another error after patching my group which only has reply attributes:

fastapi.exceptions.ResponseValidationError: 3 validation errors:
  {'type': 'missing', 'loc': ('response', 'groupname'), 'msg': 'Field required', 'input': GroupUpdate(checks=None, replies=[AttributeOpValue(attribute='Filter-Id', op=':=', value='10m')], users=None)}
  {'type': 'list_type', 'loc': ('response', 'checks'), 'msg': 'Input should be a valid list', 'input': None}
  {'type': 'list_type', 'loc': ('response', 'users'), 'msg': 'Input should be a valid list', 'input': None}

That is because there is a mismatch between the response_model which is set to Group and the object you returning which is a GroupUpdate. The same applies for User and Nas. And also in the current code, I work with empty list rather than None.

* Unit tests still not cover all the `pyfreeradius` module:

$ coverage run -m unittest test.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.143s

OK

$ coverage report
Name              Stmts   Miss  Cover
-------------------------------------
database.py           5      0   100%
pyfreeradius.py     333     16    95%
test.py             102      0   100%
-------------------------------------
TOTAL               440     16    96%

Note that I am working on the unit tests of the API itself as per Testing.

This is intentional. I've removed the username from the UserUpdate class, since the actual username is passed on in the url when making the call. If not, you could pass one username in the URL, and another in the body, and the one in the body would be ignored, since I use the one from the URL right now. This might be confusing.

Also, when patching, there is a difference between having a null or an empty list []. Passing null to the attributes for example won't update the attributes, while passing an empty list will remove all current attributes. Therefore, the defaults in the *Update objects are None instead of the [] in the original objects.

I'm not sure what I need the change for this?

angely-dev commented 4 months ago

About the username/groupname/nasname, no need to have it as attribute of the Update models (just like you did), we just need to use the query parameter.

About the null, yes I am aware of the difference with [], especially in the JSON Merge Patch strategy. I think we need to change the original behavior where I preferred to use empty lists instead of None and update the unit tests. I could do this change.

angely-dev commented 4 months ago

Before changing the original behavior, I tried this:

@router.patch('/groups/{groupname}', tags=['groups'], status_code=200, response_model=Group, responses={**e409_response})
def patch_group(groupname: str, group: GroupUpdate, response: Response):
    if not group_repo.exists(groupname):
        raise HTTPException(404, 'Given group does not exist')

    for user in group.users or []:
        if not user_repo.exists(user.username):
            raise HTTPException(422, f"Given user '{user.username}' does not exist: create it first")

    current_group = group_repo.find_one(groupname)
    if (not current_group.replies and group.replies == []) and (not current_group.checks and group.checks == []):
        raise HTTPException(422, 'Group must have at least one check or one reply attribute')

    group_repo.update(groupname, group)
    response.headers['Location'] = f'{API_URL}/groups/{groupname}'  # used groupname instead of group.groupname
    return group_repo.find_one(groupname)                           # simply re-fetch the group once updated

So it's better but I am still encountering an error easy to reproduce:

curl -X 'POST' \
  'http://localhost:8000/groups' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "groupname": "my-group",
  "replies": [
    {
      "attribute": "Filter-Id",
      "op": ":=",
      "value": "10m"
    }
  ]
}'

201 Created:

{"groupname":"my-group","checks":[],"replies":[{"attribute":"Filter-Id","op":":=","value":"10m"}],"users":[]}

Now I remove the replies, expecting an HTTPException that'd forbid it:

curl -X 'PATCH' \
  'http://localhost:8000/groups/my-group' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "replies": []
}'

Internal Server Error

Trace:

INFO:     127.0.0.1:57120 - "POST /groups HTTP/1.1" 201 Created
INFO:     127.0.0.1:42542 - "PATCH /groups/my-group HTTP/1.1" 500 Internal Server Error
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/uvicorn/protocols/http/httptools_impl.py", line 399, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 70, in __call__
    return await self.app(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/fastapi/applications.py", line 1054, in __call__
    await super().__call__(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/applications.py", line 123, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/middleware/errors.py", line 186, in __call__
    raise exc
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/middleware/errors.py", line 164, in __call__
    await self.app(scope, receive, _send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 65, in __call__
    await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app
    raise exc
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app
    await app(scope, receive, sender)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/routing.py", line 756, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/routing.py", line 776, in app
    await route.handle(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/routing.py", line 297, in handle
    await self.app(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/routing.py", line 77, in app
    await wrap_app_handling_exceptions(app, request)(scope, receive, send)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app
    raise exc
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app
    await app(scope, receive, sender)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/starlette/routing.py", line 72, in app
    response = await func(request)
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/fastapi/routing.py", line 296, in app
    content = await serialize_response(
  File "/tmp/freeradius-api-allow-updates/venv/lib/python3.10/site-packages/fastapi/routing.py", line 155, in serialize_response
    raise ResponseValidationError(
fastapi.exceptions.ResponseValidationError: 1 validation errors:
  {'type': 'model_attributes_type', 'loc': ('response',), 'msg': 'Input should be a valid dictionary or object to extract fields from', 'input': None}

I still appreciate the help and the will to enhance this projet but I am surprised the code wasn't fully tested on your side. That's why I insist about the full code covarage of pyfreeradius and soon, the full code coverage of api.py (I can add it after merging this PR).

angely-dev commented 4 months ago

Please note I just added unit tests of the API in fe4d33c. I created a tests/ directory and renamed the original test file without modifying it so that it doesn't impact your changes so much (I will modify it later to follow pytest naming convention). I also updated the README.

angely-dev / freeradius-api

Add support for PATCH to partially replace an entity #6