Share session data across mounted routes

[florimondmanca]

Fixes #1261

The resolution here builds on @devsetgo's idea:

I would suggest checking if in Mount app=None as a way to determine whether to use ASGI path or "/".

Indeed, if we Mount(routes=...), the routes actually belong to the current app in the mount hierarchy, not to a fully separate app mounted on the sub-path, so session data on the current app should be shared with them.

Note that we otherwise keep the "use root_path for (fully) sub-mounted apps" for #233.

[devsetgo]

Fixes #1261

The resolution here builds on @devsetgo's idea:

I would suggest checking if in Mount app=None as a way to determine whether to use ASGI path or "/".

Indeed, if we Mount(routes=...), the routes actually belong to the current app in the mount hierarchy, not to a fully separate app mounted on the sub-path, so session data on the current app should be shared with them.

Note that we otherwise keep the "use root_path for (fully) sub-mounted apps" for #233.

This looks like a great solution... also, I now learned how to test the session cookie.

[Kludex]

Answering your comment above: in terms of styling, either the current or using the attribute you previously had, it's fine by me.

I would've used session_path as there's no concept of "session_root_path" yet, so we can choose the name. But whatever you prefer is fine. :)

Waiting for new changes.

[Kludex]

Did you delete your comment? 🤔

[florimondmanca]

@Kludex Yes 😅 I thought about an even more specific name, pushed it in a commit. Also I think we should use the attribute style, at least check on routes explicitly. I thought about what Mount(..., app=Router(...)) should do and I guess in that case we should definitely treat it as a separate thing, like we do for any app=....

[Kludex]

LGTM! :)

[Kludex]

@florimondmanca Is there something else you'd want to check here?

[florimondmanca]

I think this is an okay change — as in, it does solve the use case. But the "special case" flavor of it still makes me think I'd like @tomchristie's opinion. Tom? 😄

[0x1997]

Any updates?

[tomchristie]

Thanks @florimondmanca! Going to close this off in favour of #1512.