Code

Opened 6 months ago

Closed 3 months ago

#21765 closed New feature (fixed)

Contexts cannot be compared for equality.

Reported by: Keryn Knight <django@…> Owned by: onjin
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, comparing two contexts for equality requires knowing the internals of how Context is implemented -- ie: that there is a .dicts attribute available on an instance. For the purposes of writing unit tests, I needed to compare a context and a mutated one, and was surprised to find there's no concept of context equality:

>>> from django.template import Context
>>> Context() == Context()
False
>>> Context().dicts == Context().dicts
True
>>> Context({'x': 'y'}).dicts == Context({'x': 'y'}).dicts
True
>>> Context({'x': 'y'}).dicts == Context({'x': 'z'}).dicts
False
>>>

It possibly doesn't make sense for Context to have the gt/ge/lt/le magic methods defined on it (though one could argue they check the length of the .dicts), but from an API standpoint it might be nice to see eq/ne implemented for clean comparisons.

Tagging with master as the context doesn't seem to have changed since 1.6.1, which is what I'm testing against.

Attachments (0)

Change History (7)

comment:1 Changed 6 months ago by mjtamlyn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Seems a reasonable request to me.

comment:2 Changed 6 months ago by Keryn Knight <django@…>

As an implementation detail, should any context subclass be considered equal if it's underlying dataset is the same, such that Context({'x': 1}) == RenderContext({'x': 1}) and vice versa, or should type(x) == type(y) in addition to x.dicts == y.dicts? It's unclear what the best course of action would be, given there's no expectation of equality except my own.

comment:3 Changed 5 months ago by onjin

  • Owner changed from nobody to onjin
  • Status changed from new to assigned

comment:4 Changed 5 months ago by Baptiste Mispelon <bmispelon@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In d97bf2e9c8971764690caaf81a0914bc368d6b02:

Fixed #21765 -- Added support for comparing Context instances

comment:5 Changed 5 months ago by Keryn Knight <django@…>

  • Resolution fixed deleted
  • Status changed from closed to new

Re-opening with permission:
The fix as committed is not entirely correct, I don't think, as it only compares the last state change of a given context, so comparisons can incorrectly return as equal if context states drift and then re-align, for example:

>>> import django
>>> from django.template import Context
>>> a = Context()
>>> b = Context()
>>> a == b
True

the above is correct; from here on out, the contexts are continuations of the above example:

>>> a.update({'a': 1})
>>> a == b
False

that's also correct.

>>> a.update({'c': 3})
>>> b.update({'c': 3})
>>> a == b
True

This is where the incorrectness appears. They are not equal, but their last push to the stack was, so they're being treated as equal. Based on the commit in question (d97bf2e9c8971764690caaf81a0914bc368d6b02), the change would be going from:

return self.dicts[-1] == other.dicts[-1]

to

return self.dicts == other.dicts

which would at least ensure the rest of the context is taken into account, though that fix in itself may not be as correct as it should be -- it is strict in the sense that the entire history of the contexts being compared must be aligned, where it should perhaps instead squash the stack into one dictionary for both sides and compare them for equality.

(Whether or not strictness is a benefit or problem is probably a DDN, and like testing equality for context subclasses which differ (or have different attrs, for things like RenderContext) could probably be tracked in separate tickets, though.)

comment:6 Changed 5 months ago by Baptiste Mispelon <bmispelon@…>

In 8274fa60f88607eb0e81908f049c391455956dd8:

Made the new template.Context.flatten() method a public API.

That method was introduced in 9db4271bd11ac23a5a5652bbcdf8fb6d4b997651.

Refs #21765.

comment:7 Changed 3 months ago by leotin

  • Resolution set to fixed
  • Status changed from new to closed

It seems like the reported issue is finally fixed after the update by 8274fa60f88607eb0e81908f049c391455956dd8.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.