Feature/11 combine profile update and fi association #38

guffee23 · 2023-09-26T19:01:27Z

Closes #11

Added UserProfile dto. Abstracted lei association method. Added lei association to admin update user api.

…ociation

Updated json for all related calls. Changed update test to incorporate association testing. Added test where leis not present in call. Updated associate api test.

…ociation

github-actions · 2023-09-26T19:11:08Z

Coverage report

The coverage rate went from 84.32% to 83.91% ⬇️
The branch rate is 84%.

90% of new lines are covered.

Diff Coverage details (click to unfold)

src/entities/models/init.py

100% of new lines are covered (100% of the complete file).

src/entities/models/dto.py

100% of new lines are covered (100% of the complete file).

src/oauth2/oauth2_admin.py

50% of new lines are covered (33.33% of the complete file).
Missing lines: 92, 93

src/routers/admin.py

100% of new lines are covered (100% of the complete file).

lchen-2101 · 2023-09-27T16:25:51Z

src/entities/models/dto.py

+    firstName: str
+    lastName: str


let's keep the variable naming to the python convention of snake case, and use alias_generator to convert them to camel case: https://docs.pydantic.dev/latest/api/config/#pydantic.alias_generators.to_camel
https://stackoverflow.com/questions/67995510/how-to-inflect-from-snake-case-to-camel-case-post-the-pydantic-schema-validation

The body was to look like:
{ "firstName": "First", "lastName": "Last", "leis": ["LEI1", "LEI2"] }
So, we need aliasing for the request as well as when generating the dict?
To match the request reqs and the syntax that keycloak expects, we would need it in the form above, but to change to snake case would require aliasing to snake when request is sent and when creating dict being sent to keycloak.
Just double-checking here. Should the request body be changed to snake case?

request body to the api should be snake case, we would then alias it to camel case for keycloak.

Just so I understand, our API's User resource will look like:

{ "first_name":"Jane", "last_name":"Doe", "leis": ["123456789012","234567890123"], }

Meanwhile, the Keycloak Admin API's UserRepresentation expects:

{ "firstName": "Jane", "lastName": "Doe" }

...and to associated LEIs to said User, it's reeeaaally associating a Keycloak User to a Keycloak Group which is named after a given LEI? So, to complete the transaction, and second call that's made to...

PUT /admin/realms/{realm}/users/{id}/groups/{groupId}

If yes, 👍. If no, I may need some help here. 😅

I think what @lchen-2101 is getting at is that we shouldn't couple our API format to Keycloak's, which I agree with.

As to Pydantic's to_camel, I'm not sure we want to do that. There's only 2 attributes we care about. Aaaand, what happens to leis on our model when you do the to_camel? Wouldn't that still be there in the data we'd be submitting to Keycloak? Seems like that might break.

To me, it doesn't seem that gross to just construct the dict at time of request in update_me(). 🤷 Looks like we're already doing it that way in upsert_group now anyway.

lchen-2101 · 2023-09-27T16:29:25Z

src/oauth2/oauth2_admin.py

@@ -88,5 +88,9 @@ def associate_to_lei(self, user_id: str, lei: str) -> None:
                detail="No institution found for given LEI",
            )

+    def associate_to_lei_set(self, user_id, leis: Set[str]):


normally if we can, we'd like to keep the implementation detail of the collection out of the var name / method name; this one should be good with just associate_to_leis

lchen-2101 · 2023-09-27T17:00:48Z

src/entities/models/dto.py

+    def get_user(self) -> Dict[str, Any]:
+        return {"firstName": self.firstName, "lastName": self.lastName}


pydantic should have something built-in that will generate the dict for you, I don't think you need to create a separate method for this.

hkeeler · 2023-09-27T22:54:32Z

src/routers/admin.py

@@ -19,12 +20,13 @@ async def get_me(request: Request):

 @router.put("/me/", status_code=HTTPStatus.ACCEPTED, dependencies=[Depends(check_domain)])
 @requires("manage-account")
-async def update_me(request: Request, user: Dict[str, Any]):
-    oauth2_admin.update_user(request.user.id, user)
+async def update_me(request: Request, user: UserProfile):


Should API functions that use the Keycloak client be async? It looks like python-keycloak uses requests under the hood, which is not async. I think they'd need to use something like httpx to support that. As it stands now, I think this could lead to unintentional blocking during these calls.

Dropping async is not a bad thing. FastAPI will just run these requests in a new thread, which I think is what we'd want for synchronous processes like this.

hkeeler · 2023-09-28T00:12:11Z

src/entities/models/dto.py

+    firstName: str
+    lastName: str


Just so I understand, our API's User resource will look like:

{ "first_name":"Jane", "last_name":"Doe", "leis": ["123456789012","234567890123"], }

Meanwhile, the Keycloak Admin API's UserRepresentation expects:

{ "firstName": "Jane", "lastName": "Doe" }

...and to associated LEIs to said User, it's reeeaaally associating a Keycloak User to a Keycloak Group which is named after a given LEI? So, to complete the transaction, and second call that's made to...

PUT /admin/realms/{realm}/users/{id}/groups/{groupId}

If yes, 👍. If no, I may need some help here. 😅

I think what @lchen-2101 is getting at is that we shouldn't couple our API format to Keycloak's, which I agree with.

As to Pydantic's to_camel, I'm not sure we want to do that. There's only 2 attributes we care about. Aaaand, what happens to leis on our model when you do the to_camel? Wouldn't that still be there in the data we'd be submitting to Keycloak? Seems like that might break.

To me, it doesn't seem that gross to just construct the dict at time of request in update_me(). 🤷 Looks like we're already doing it that way in upsert_group now anyway.

Updated dto to snake case. Changed associate lei method name. Utilized pydantic dictionaries. Updated tests to account for method name change.

…ociation

lchen-2101 · 2023-10-03T20:00:29Z

src/entities/models/dto.py

+class UserProfile(BaseModel):
+    first_name: str = Field(alias="firstName")
+    last_name: str = Field(alias="lastName")
+    leis: Optional[Set[str]] = Field(alias="leis")
+
+    class Config:
+        alias_generator = to_camel


found a couple of things with the configs, the to_camel generator is actually doing pascal case; there is a to_lower_camel which would achieve the goal, without you having to specify Field; there is also an allow_population_by_field_name option which would allow the request payload to be snake case instead of just camel case; while it doesn't change the swagger doc to show camel case, it does allow the API to accept both camel case and snake case. So what you end up with would be:

class UserProfile(BaseModel): first_name: str last_name: str leis: Optional[Set[str]] class Config(BaseConfig): alias_generator = to_lower_camel allow_population_by_field_name = True

just a side note; when we update fastapi later, the included pydantic package also changes to 2.x from 1.x, and some of these configurations will be changed. Nothing to worry about here, but in the pydantic settings issue, the dependency will be updated, so more changes will happen here as well.

Why do we need/want to have our API accept camelCase? Is that just an artifact of us using the same UserProfile model for both our API and for submitting to Keycloak's API? To me it seems more explicit and future-proof to decouple the two, and just do a simple translation to the expected JSON format at the time we call the Keycloak API. If we go this route, we have one oddball model object with extra Config section that likely needs an explainer comment for what it's doing there, and we'll need to keep the exclude={"leis"} bit up-to-date every time you add another field UserProfile.

This is all what I was getting at when I asked #38 (comment), so it's also possible I'm just missing something.

@hkeeler It has to do with keycloak and the dictionary vals it's expecting, which are in camelCase.

I get that. My question is, why are we doing these Pydantic gymnastics when we could just...

def update_me(request: Request, user: UserProfile): keycloak_user = {'firstName': user.first_name, 'lastName': user.last_name} oauth2_admin.update_user(request.user.id, keycloak_user) if user.leis: oauth2_admin.associate_to_leis(request.user.id, user.leis)

my thought would be still to have a well defined model to communicate with Keycloak instead of constructing a dict on the spot. I was hoping to leverage what's built into pydantic, and having the config take care of SerDe; but if we want to do away with the configs, having a method on the model would be more preferable.

But that model is for our API, not Keycloak's, right? It feels like we're using the fact that the two APIs happen to be similar enough in message structure (currently) that we're using the same model for both, but doing so comes with the baggage of having their fields now coupled, having to understand/explain why one model is different from the rest, and having to adjust a disconnected method to exclude future fields we add.

Now, we could have a dedicated model for calls to Keycloak. I mean really, that should be the job of python-keycloak, but that seems to just be a very a light wrapper around their API. I think that's probably more than we need for this PR, though.

For now, I agree having a to_keycloak_user() on the user model works. It's simple and explicit.

tests/api/routers/test_admin_api.py

hkeeler · 2023-10-04T16:52:51Z

src/oauth2/oauth2_admin.py

@@ -88,5 +88,9 @@ def associate_to_lei(self, user_id: str, lei: str) -> None:
                detail="No institution found for given LEI",
            )

+    def associate_to_leis(self, user_id, leis: Set[str]):


Suggested change

def associate_to_leis(self, user_id, leis: Set[str]):

def associate_to_leis(self, user_id: str, leis: Set[str]):

hkeeler · 2023-10-04T17:36:27Z

src/entities/models/dto.py

+class UserProfile(BaseModel):
+    first_name: str = Field(alias="firstName")
+    last_name: str = Field(alias="lastName")
+    leis: Optional[Set[str]] = Field(alias="leis")
+
+    class Config:
+        alias_generator = to_camel


Why do we need/want to have our API accept camelCase? Is that just an artifact of us using the same UserProfile model for both our API and for submitting to Keycloak's API? To me it seems more explicit and future-proof to decouple the two, and just do a simple translation to the expected JSON format at the time we call the Keycloak API. If we go this route, we have one oddball model object with extra Config section that likely needs an explainer comment for what it's doing there, and we'll need to keep the exclude={"leis"} bit up-to-date every time you add another field UserProfile.

This is all what I was getting at when I asked #38 (comment), so it's also possible I'm just missing something.

hkeeler · 2023-10-05T16:38:33Z

src/entities/models/dto.py

+    class Config:
+        allow_population_by_field_name = True
+
+


Can this be removed now too?

hkeeler

👍

lchen-2101

LGTM

guffee23 added 5 commits September 26, 2023 12:32

Changed admin update user api to associate lei

f90c4f1

Added UserProfile dto. Abstracted lei association method. Added lei association to admin update user api.

Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…

2ed0d01

…ociation

Updated admin update test to include association

a1453c0

Updated json for all related calls. Changed update test to incorporate association testing. Added test where leis not present in call. Updated associate api test.

Linting

e16b624

Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…

1057704

…ociation

lchen-2101 reviewed Sep 27, 2023

View reviewed changes

hkeeler reviewed Sep 28, 2023

View reviewed changes

guffee23 added 5 commits September 28, 2023 11:49

Various formatting changes

face017

Updated dto to snake case. Changed associate lei method name. Utilized pydantic dictionaries. Updated tests to account for method name change.

Black linting

fe0a473

Removed unused line

562a06f

Changed admin apis from async to sync

086a75f

Merge branch 'main' into Feature/11_combine_profile_update_and_fi_ass…

80861ee

…ociation

lchen-2101 reviewed Oct 3, 2023

View reviewed changes

Updated User Profile dto and tests

ff582cb

lchen-2101 reviewed Oct 4, 2023

View reviewed changes

tests/api/routers/test_admin_api.py Show resolved Hide resolved

hkeeler reviewed Oct 4, 2023

View reviewed changes

guffee23 added 3 commits October 4, 2023 14:01

Updated no lei test and associate func params

df3522f

Updated dto and admin update api

d5a746c

Linting

5a75928

hkeeler reviewed Oct 5, 2023

View reviewed changes

Removed user profile dto config

9e6d2aa

hkeeler approved these changes Oct 5, 2023

View reviewed changes

lchen-2101 approved these changes Oct 5, 2023

View reviewed changes

lchen-2101 merged commit 0c9ef87 into main Oct 5, 2023

lchen-2101 deleted the Feature/11_combine_profile_update_and_fi_association branch October 5, 2023 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/11 combine profile update and fi association #38

Feature/11 combine profile update and fi association #38

guffee23 commented Sep 26, 2023

github-actions bot commented Sep 26, 2023 •

edited

Loading

src/entities/models/init.py

src/entities/models/dto.py

src/oauth2/oauth2_admin.py

src/routers/admin.py

lchen-2101 Sep 27, 2023

guffee23 Sep 27, 2023

lchen-2101 Sep 27, 2023

hkeeler Sep 28, 2023

lchen-2101 Sep 27, 2023

lchen-2101 Sep 27, 2023

hkeeler Sep 27, 2023

hkeeler Sep 28, 2023

lchen-2101 Oct 3, 2023 •

edited

Loading

lchen-2101 Oct 3, 2023

hkeeler Oct 4, 2023

guffee23 Oct 4, 2023

hkeeler Oct 4, 2023

lchen-2101 Oct 4, 2023

hkeeler Oct 4, 2023

hkeeler Oct 4, 2023

hkeeler Oct 4, 2023

hkeeler Oct 5, 2023

guffee23 Oct 5, 2023

hkeeler left a comment

lchen-2101 left a comment

		def get_user(self) -> Dict[str, Any]:
		return {"firstName": self.firstName, "lastName": self.lastName}

	def associate_to_leis(self, user_id, leis: Set[str]):
	def associate_to_leis(self, user_id: str, leis: Set[str]):

Feature/11 combine profile update and fi association #38

Feature/11 combine profile update and fi association #38

Conversation

guffee23 commented Sep 26, 2023

github-actions bot commented Sep 26, 2023 • edited Loading

Coverage report

src/entities/models/init.py

src/entities/models/dto.py

src/oauth2/oauth2_admin.py

src/routers/admin.py

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchen-2101 Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkeeler left a comment

Choose a reason for hiding this comment

lchen-2101 left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2023 •

edited

Loading

lchen-2101 Oct 3, 2023 •

edited

Loading