feat: Changes suggested in feedback

Author: KShivendu

supertokens_python/recipe/session/session_class.py

@@ -12,14 +12,14 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 import json
-from typing import Any, Dict, Union, List, TypeVar
+from typing import Any, Dict, List, TypeVar, Union
 
 from supertokens_python.recipe.session.exceptions import (
-    raise_unauthorised_exception,
     raise_invalid_claims_exception,
+    raise_unauthorised_exception,
 )
 
-from .interfaces import SessionContainer, SessionClaimValidator, SessionClaim
+from .interfaces import SessionClaim, SessionClaimValidator, SessionContainer
 from .utils import update_claims_in_payload_if_needed, validate_claims_in_payload
 
 _T = TypeVar("_T")
@@ -154,6 +154,7 @@ async def fetch_and_set_claim(
     ) -> None:
         if user_context is None:
             user_context = {}
+
         update = await claim.build(self.get_user_id(), user_context)
         return await self.merge_into_access_token_payload(update, user_context)
 
@@ -172,6 +173,9 @@ async def set_claim_value(
     async def get_claim_value(
         self, claim: SessionClaim[Any], user_context: Union[Dict[str, Any], None] = None
     ) -> Union[Any, None]:
+        if user_context is None:
+            user_context = {}
+
         return claim.get_value_from_payload(
             self.get_access_token_payload(user_context), user_context
         )
@@ -181,12 +185,18 @@ async def remove_claim(
     ) -> None:
         if user_context is None:
             user_context = {}
+
         update = claim.remove_from_payload_by_merge_({}, user_context)
         return await self.merge_into_access_token_payload(update, user_context)
 
     async def merge_into_access_token_payload(
-        self, access_token_payload_update: Dict[str, Any], user_context: Dict[str, Any]
+        self,
+        access_token_payload_update: Dict[str, Any],
+        user_context: Union[Dict[str, Any], None] = None,
     ) -> None:
+        if user_context is None:
+            user_context = {}
+
         update_payload = {
             **self.get_access_token_payload(user_context),
             **access_token_payload_update,

supertokens_python/recipe/session/utils.py

@@ -552,7 +552,7 @@ async def validate_claims_in_payload(
         log_debug_message(
             "validate_claims_in_payload %s validate res %s",
             validator.id,
-            json.dumps(claim_validation_res),
+            json.dumps(claim_validation_res.__dict__),
         )
         if (
             not claim_validation_res.is_valid

tests/sessions/claims/test_fetch_and_set_claim.py

@@ -26,7 +26,7 @@ async def test_should_not_change_if_claim_fetch_value_returns_none():
         wraps=session.merge_into_access_token_payload,
     ) as mock:
         await session.fetch_and_set_claim(NoneClaim)
-        mock.assert_called_once_with({}, None)
+        mock.assert_called_once_with({}, {})
 
 
 async def test_should_update_if_claim_fetch_value_returns_value(timestamp: int):
@@ -44,4 +44,4 @@ async def test_should_update_if_claim_fetch_value_returns_value(timestamp: int):
         wraps=session.merge_into_access_token_payload,
     ) as mock:
         await session.fetch_and_set_claim(TrueClaim)
-        mock.assert_called_once_with({"st-true": {"t": timestamp, "v": True}}, None)
+        mock.assert_called_once_with({"st-true": {"t": timestamp, "v": True}}, {})

tests/sessions/claims/test_primitive_claim.py

@@ -99,13 +99,11 @@ async def test_validators_should_not_validate_empty_payload():
     claim = PrimitiveClaim("key", sync_fetch_value)
     res = await claim.validators.has_value(val).validate({}, {})
 
-    assert res == {
-        "isValid": False,
-        "reason": {
-            "expectedValue": val,
-            "actualValue": None,
-            "message": "wrong value",
-        },
+    assert res.is_valid is False
+    assert res.reason == {
+        "expectedValue": val,
+        "actualValue": None,
+        "message": "wrong value",
     }
 
 
@@ -114,13 +112,11 @@ async def test_should_not_validate_mismatching_payload():
     payload = await claim.build("user_id")
     res = await claim.validators.has_value(val2).validate(payload, {})
 
-    assert res == {
-        "isValid": False,
-        "reason": {
-            "expectedValue": val2,
-            "actualValue": val,
-            "message": "wrong value",
-        },
+    assert res.is_valid is False
+    assert res.reason == {
+        "expectedValue": val2,
+        "actualValue": val,
+        "message": "wrong value",
     }
 
 
@@ -129,7 +125,7 @@ async def test_validator_should_validate_matching_payload():
     payload = await claim.build("user_id")
     res = await claim.validators.has_value(val).validate(payload, {})
 
-    assert res == {"isValid": True}
+    assert res.is_valid is True
 
 
 async def test_should_validate_old_values_as_well(patch_get_timestamp_ms: MagicMock):
@@ -140,7 +136,7 @@ async def test_should_validate_old_values_as_well(patch_get_timestamp_ms: MagicM
     patch_get_timestamp_ms.return_value += 100  # type: ignore
 
     res = await claim.validators.has_value(val).validate(payload, {})
-    assert res == {"isValid": True}
+    assert res.is_valid is True
 
 
 async def test_should_refetch_if_value_not_set():
@@ -165,36 +161,32 @@ async def test_validator_should_not_refetch_if_value_is_set():
 async def test_should_not_validate_empty_payload():
     claim = PrimitiveClaim("key", sync_fetch_value)
     res = await claim.validators.has_fresh_value(val, 600).validate({}, {})
-    assert res == {
-        "isValid": False,
-        "reason": {
-            "expectedValue": val,
-            "actualValue": None,
-            "message": "value does not exist",  # TODO: Validate that this is actually correct.
-            # because this makes sense yet the node PR isn't aligned with this.
-        },
+    assert res.is_valid is False
+    assert res.reason == {
+        "expectedValue": val,
+        "actualValue": None,
+        "message": "value does not exist",  # TODO: Validate that this is actually correct.
+        # because this makes sense yet the node PR isn't aligned with this.
     }
 
 
 async def test_has_fresh_value_should_not_validate_mismatching_payload():
     claim = PrimitiveClaim("key", sync_fetch_value)
     payload = await claim.build("user_id")
     res = await claim.validators.has_fresh_value(val2, 600).validate(payload, {})
-    assert res == {
-        "isValid": False,
-        "reason": {
-            "expectedValue": val2,
-            "actualValue": val,
-            "message": "wrong value",
-        },
+    assert res.is_valid is False
+    assert res.reason == {
+        "expectedValue": val2,
+        "actualValue": val,
+        "message": "wrong value",
     }
 
 
 async def test_should_validate_matching_payload():
     claim = PrimitiveClaim("key", sync_fetch_value)
     payload = await claim.build("user_id")
     res = await claim.validators.has_fresh_value(val, 600).validate(payload, {})
-    assert res == {"isValid": True}
+    assert res.is_valid is True
 
 
 async def test_should_not_validate_old_values_as_well(
@@ -208,13 +200,11 @@ async def test_should_not_validate_old_values_as_well(
     patch_get_timestamp_ms.return_value += 100 * SECONDS  # type: ignore
 
     res = await claim.validators.has_fresh_value(val, 10).validate(payload, {})
-    assert res == {
-        "isValid": False,
-        "reason": {
-            "ageInSeconds": 100,
-            "maxAgeInSeconds": 10,
-            "message": "expired",
-        },
+    assert res.is_valid is False
+    assert res.reason == {
+        "ageInSeconds": 100,
+        "maxAgeInSeconds": 10,
+        "message": "expired",
     }
 
 

tests/sessions/claims/test_set_claim_value.py

@@ -37,7 +37,7 @@ async def test_should_merge_the_right_value(timestamp: int):
         wraps=session.merge_into_access_token_payload,
     ) as mock:
         await session.set_claim_value(TrueClaim, False)
-        mock.assert_called_once_with({"st-true": {"t": timestamp, "v": False}}, None)
+        mock.assert_called_once_with({"st-true": {"t": timestamp, "v": False}}, {})
 
 
 async def test_should_overwrite_claim_value(timestamp: int):
@@ -54,7 +54,7 @@ async def test_should_overwrite_claim_value(timestamp: int):
 
     # Payload should be updated now:
     payload = s.get_access_token_payload()
-    assert payload == {"st-true": {"t": timestamp, "v": "NEW_TRUE"}}
+    assert payload == {"st-true": {"t": timestamp, "v": False}}
 
 
 async def test_should_overwrite_claim_value_using_session_handle(timestamp: int):

tests/sessions/claims/test_verify_session.py

@@ -327,7 +327,7 @@ async def test_should_reject_if_assert_claims_returns_an_error(
             [
                 ClaimValidationError(
                     "test_id",
-                    {"msg": "test_reason"},
+                    {"message": "test_reason"},
                 )
             ],
         )

tests/sessions/claims/test_with_jwt.py

@@ -42,7 +42,6 @@ async def create_api(request: Request):  # type: ignore
 async def test_should_create_the_right_access_token_payload_with_claims_and_JWT_enabled(
     fastapi_client: TestClient,
 ):
-    # TODO: FIXME
     init(**get_st_init_args(TrueClaim, jwt=JWTConfig(enable=True)))  # type:ignore
     start_st()
 
@@ -52,7 +51,6 @@ async def test_should_create_the_right_access_token_payload_with_claims_and_JWT_
     session_info = await get_session_information(session_handle)
     assert session_info is not None
     access_token_payload = session_info.access_token_payload
-    # TODO: .sub and .iss should be undefined as per node PR
     assert access_token_payload["jwt"] is not None
     assert access_token_payload["_jwtPName"] == "jwt"