Fix unexpected "Vary"

[jrobichaud]

Closes #734

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d426b55) to head (d97d691).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #735   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           51        51           
  Lines         2050      2116   +66     
  Branches        31        33    +2     
=========================================
+ Hits          2050      2116   +66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[last-partizan]

While this is working as expected, I'm not sure this is the best way to go, because we're accessing user and then lying to SessionMiddleware basically.

I see two possible alternatives here.

1. Use truly lazy user_id, something along these lines:

    @staticmethod
    def bind_user_id(request: "HttpRequest") -> None:
        class LazyUserId:
            def __repr__(self):
                user_id_field = app_settings.USER_ID_FIELD
                user_id = None
                if hasattr(request, "user") and request.user is not None and user_id_field:
                    if hasattr(request.user, user_id_field):
                        user_id = getattr(request.user, user_id_field)
                return str(user_id)

        structlog.contextvars.bind_contextvars(user_id=LazyUserId())

If log level is higher than info, these variables (logger.info("request_started", **log_kwargs)) aren't used, so we do not access user_id at all. No Vary: Cookie.

But, with log level <= INFO, those logs are written, and so Vary: Cookie added.

2. Check if request.session was already accessed, and add user_id only in this case.

    @staticmethod
    def bind_user_id(request: "HttpRequest") -> None:
        user_id_field = app_settings.USER_ID_FIELD
        if (
            hasattr(request, "session")
            and request.session.accessed
            and hasattr(request, "user")
            and request.user is not None
            and user_id_field
        ):
            user_id = None
            if hasattr(request.user, user_id_field):
                user_id = getattr(request.user, user_id_field)
                if isinstance(user_id, uuid.UUID):
                    user_id = str(user_id)
            structlog.contextvars.bind_contextvars(user_id=user_id)

That's also lying but from another angle - request was authorized, but it does not matter because user was not accessed.

---

I would prefer first option, because it's truly lazy and reflects reality as close as possible.

But, if you think modifying request.session.accessed is safe approach - it's good enough for me. Request has no vary: Cookie and that is what matters most.

After we agree on the best approach here, let me check this one more time in real app scenario, and I'll report back how it works.

[jrobichaud]

While this is working as expected, I'm not sure this is the best way to go, because we're accessing user and then lying to SessionMiddleware basically.

The purpose of Vary is to tell if the content returned may change for caching purpose.

Normally checking if the user has been accessed is a good way to tell if the content may change. Ex: a condition to render something based on the user, checking permissions, etc.

However django-structlog accessing the user won't affect the template at all. django-structlog needs to tell which user accessed a route even if the user was not accessed in the view.

[last-partizan]

Fair point. I have tested this code and it works as expected.

I had to make some minor changes, to catch a case when this was included before AuthenticationMiddleware, and my final version looks like this:

    @staticmethod
    def bind_user_id(request: "HttpRequest") -> None:
        user_id_field = app_settings.USER_ID_FIELD
        if not user_id_field:
            return

        if not hasattr(request, "user"):
            return

        session_was_accessed = (
            request.session.accessed if hasattr(request, "session") else None
        )

        if request.user is not None:
            user_id = None
            if hasattr(request.user, user_id_field):
                user_id = getattr(request.user, user_id_field)
                if isinstance(user_id, uuid.UUID):
                    user_id = str(user_id)
            structlog.contextvars.bind_contextvars(user_id=user_id)

        if session_was_accessed is False:
            """using SessionMiddleware but user was never accessed, must reset accessed state"""

            user = request.user

            def get_user() -> Any:
                request.session.accessed = True
                return user

            request.user = cast("AbstractBaseUser", SimpleLazyObject(get_user))
            request.session.accessed = False