context anti-patterns
- Dominant language
- No language data
- Stars
- 39
- Forks
- 1
- PR merge metrics
- No merged PRs in 30d
Description
The context dict that is passed to action functions when called locally or from controllers is often abused. I can see the argument for some kind of object that includes security-sensitive values like the user object or values like model that could let you mock out the real models for tests.
However, we have at least three classes of values that we shouldn't be passing as part of a "context":
1. using context instead of normal parameters for calls from an action function to other parts of ckan. e.g. the 'prevent_packages_update' passed to group_dict save (which I'd fix this way: https://github.com/wardi/ckan/commit/303ab34354dafd214144ff49117df4403ce9174a )
There's no reason at all I can see for this sort of use. it's a value used by the immediate function we're calling and only that function. That's what parameters are for. Python even lets you set a default and has conventions for documenting these things.
2. using context to pass non-sensitive values many levels to where they are actually used. e.g. the revision_id/revision_date values that can be passed to package_show https://github.com/ckan/ckan/blob/master/ckan/controllers/package.py#L356-L372
This is an actually-useful parameter that should be part of the package_show API! Why is it being hidden in the context object?
When we use context to pass a value many layers deep it's very hard to reason about the behaviour of code without understanding _everything_ above and below. Let's add revision_date to package_show's data_dict, and use a normal parameter on the way down.
3. In some places because context is a mutable object it's also used to pass values _up_ to callers. e.g. adding a package object for use by a the REST api controller after package_create: https://github.com/ckan/ckan/blob/master/ckan/logic/action/create.py#L197-201
This sort of thing has led to a number of bugs myself and others have had to fix The tendency to reuse a context between many call_action calls makes this even worse. I had to help a co-workers debug an issue where some unrelated action calls would work when done in one order but not another that was caused by this.
This also makes it really hard to reason about what an action call will do. You need to know everything that might have ever been put in a context object by anyone above, below or in code that happened to run before the code you're looking at.
My preference would be to make context an immutable object like a NamedTuple that can only ever have a few expected values: e.g. model, session, user. Because it's immutable you're forced by the language to create a new one any time you need to change something, and it's always safe to re-use from one call to the next.
Such an object could mostly maintain backwards compatibility by defining a `__getattr__` method that returns the expected attributes.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.