Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.0 Feature Suggestion: Extending the ISession API to facilitate saving changed items #2694

Closed
jvanasco opened this issue Jul 11, 2016 · 8 comments

Comments

@jvanasco
Copy link
Contributor

Sorry for posting in the other thread. I thought it was "start there, then thread if appropriate"

Continued from: #2362 (comment)

Referencing: ericrasmussen/pyramid_redis_sessions#72 (which shows a version of mmerickel's test case)

Quoting mmerickel

This is up to the session whether this is required. Under normal scenarios it is only required if you perform a deep change on a mutable collection stored in the session, for obvious reasons.

Pyramid of the few frameworks around (and only one i've used of many) where Session components do not normally handle/detect this type of change. Because of how the session api itself is designed, this type of change can not be (automatically) detected.

The ways to detect these types of changes would be:

  • compare serialized versions of incoming session and finished session
  • use objects that can track changes in nested dicts

The first method can be implemented rather easily, and might be a better approach to handling sessions than the current methods.

To be clear: I use a custom library to handles this. I think the benefit would be on new developers who won't get caught by this idiosyncrasy, and that it's worth ensuring all Pyramid users don't need to think about specifics and details of sessions like this.

@mmerickel
Copy link
Member

Again, this isn't an issue with the ISession API but rather what your session impl is willing to track. SQLAlchemy suffers from this exact same problem in their JSON fields unless you explicitly configure their MutableMapping objects which can track changes in nested objects and invoke the changed machinery automatically for you.

Comparing serialized versions is not crazy, but it does require a decent performance hit per-request to check for those changes and thus would not be something I think we'd ever enable by default.

@jvanasco
Copy link
Contributor Author

I consider this an issue with the ISession API, because the Session specification states that nested value changes are not required to persist. This is a design decision for Sessions somewhat particular to Pyramid - it is not an approach that happens in most other web frameworks or languages.

SqlAlchemy does have a similar issue -- but the context is a specialized column type that was (until recently) only on one database; and the core library ships a supporting extension to enable the functionality.

Sessions are a core component of Web development. Not guaranteeing to save nested data is something that one familiar with the industry would expect as an opt-in enhancement -- not the default behavior.

@mmerickel
Copy link
Member

That's not true about it being a special case in SQLAlchemy. It's an issue for many of its data types including people marshalling anything into a Text column or LargeBinary column - ignoring the obvious XML and JSON data types which don't support it. Not receiving notifications when you change a deeply nested object is incredibly common in programming and expecting anything else is wishful thinking. At any rate i'll sum up my thoughts by saying I'm -1 on modifying the ISession API to require support for nested objects.

@digitalresistor
Copy link
Member

None of my sessions have deeply nested information... and I am a -1 on adding overhead for all users of sessions.

@mmerickel
Copy link
Member

Sessions are pickle-able content. You're basically asking for any arbitrary Python object to support calling back to the session to notify it when the object changes. It's just not feasible. It's hard enough with dicts and arrays in json. And, as @bertjwregeer said, it's never free to wrap all of the objects in proxies.

@jvanasco
Copy link
Contributor Author

SqlAlchemy's usage of mutation tracking in JSON is a special case - that object type is natively a nested structure in the database that is cast into a Python object; the extension is used to maintain parity on edits. The other two examples you gave are of developers using the extension to marshall nested Python objects into a column.

I understand your points above, however mine does not seem to come across at all – you've made a design decision with the ISession API to model it after a Key/Value store and not common web sessions. I understand that there are potential overhead issues, but this is something that "session" implementations in most languages & frameworks took in mind while designing their implementations. I understand all the concerns of deeply session objects you are bringing up, but sessions are not just a common or arbitrary deeply nested object, they are a specific application that has common uses and expectations.

I obviously don't expect this to change now, but I hope you can understand that the implementation of sessions under pyramid is simply not standard when compared to other frameworks and it operates more like a common key-value store.

@digitalresistor
Copy link
Member

There's something bothering me about the statement "No other frameworks work like this", yet I want to bring your attention to this point:

https://docs.djangoproject.com/en/1.9/topics/http/sessions/#when-sessions-are-saved

Specifically, let me quote it here for posterities sake:

When sessions are saved

By default, Django only saves to the session database when the session has been modified – that is if any of its dictionary values have been assigned or deleted:

# Session is modified.
request.session['foo'] = 'bar'

# Session is modified.
del request.session['foo']

# Session is modified.
request.session['foo'] = {}

# Gotcha: Session is NOT modified, because this alters
# request.session['foo'] instead of request.session.
request.session['foo']['bar'] = 'baz'

In the last case of the above example, we can tell the session object explicitly that it has been modified by setting the modified attribute on the session object:

request.session.modified = True

To change this default behavior, set the SESSION_SAVE_EVERY_REQUEST setting to True. When set to True, Django will save the session to the database on every single request.

Note that the session cookie is only sent when a session has been created or modified. If SESSION_SAVE_EVERY_REQUEST is True, the session cookie will be sent on every request.

Similarly, the expires part of a session cookie is updated each time the session cookie is sent.

The session is not saved if the response’s status code is 500.

So in another, very popular web framework, you have to manually tell the session that it has been updated (much like you have to call changed() in Pyramid) when you update a deeply nested object, and there is no automatic logic to try and detect it.


With that out of the way, and seeing as how that is a "common web session" I'd like to hear what exactly a "common web session" is to you. What framework/tool are you referring to? You have made vague statements but provided no evidence or proof to back it up.

What changes do you want to see to the ISession API? How exactly do you expect deeply nested objects to be tracked for changes without invasively modifying each and every single one?

Solutions such as hash all keys and compare with previous hashes are not valid answers because of the overhead associated with it (even if nothing has changed, you have to hash the contents when you re-create the session for the current request, and when the request is completed), although with the current ISession API you are absolutely free to implement that yourself, and I would encourage you to do so.

@mmerickel
Copy link
Member

The current API leaves this an open problem and any implementation of ISession is free to choose how they detect changes, so long as they also support the changed() call. It's totally valid to write an ISession that compares hashes, or always saves changes similar to django. For that reason, and all the reasons above, I don't see Pyramid modifying the core API to require top-level support for such a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants