fix: Update group dependencies for new rfc model #6773
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6773 +/- ##
=======================================
Coverage 88.80% 88.81%
=======================================
Files 285 285
Lines 40280 40287 +7
=======================================
+ Hits 35771 35779 +8
+ Misses 4509 4508 -1 ☔ View full report in Codecov by Sentry. |
ietf/group/views.py
Outdated
| @@ -737,26 +737,35 @@ def dependencies(request, acronym, group_type=None): | |||
|
|
|||
| references = Q( | |||
| Q(source__group=group) | Q(source__in=cl_docs), | |||
| source__type="draft", | |||
| source__type="draft", # does this need to be type__in=["draft", "rfc"]? Check for informative ref rfc->draft | |||
There was a problem hiding this comment.
I think likely not, but it might be a change from previous behavior without it.
Since this is only used for groups, we don't have to worry about rfcs that didn't come from drafts here.
The only use such data might have to the consumer of this function is knowing that some RFC inf-reffed this group's active draft. But that's a done deal, so the group won't really be motivated to do anything with the draft given this knowledge. So it's probably noise and shouldn't be on the graph in the first place.
There was a problem hiding this comment.
I looked at a handful of cases comparing against the 11.16.0 release (i.e., for posterity, the release before the one with the "new" rfc modeling) and did not see any where nodes were missing. That included some selected specifically because they had a refinfo relationship pointing from an RFC to a group draft.
Also, I think the only way this could miss a relationship is if there was a relationship from an RFC to a group draft without there also having been a relationship from the draft that became the RFC to the group draft. Does that happen (aside from April 1 RFCs, which I feel pretty comfortable leaving off these plots except perhaps for amusement value)?
| "expired": x.get_state("draft").slug == "expired", | ||
| "replaced": x.get_state("draft").slug == "repl", | ||
| "group": x.group.acronym if x.group.acronym != "none" else "", | ||
| "id": x.became_rfc().name if x.became_rfc() else x.name, |
There was a problem hiding this comment.
Check this on a busy group and if the time is noticeable, I think we can get the db to grab this with the primary query instead of n+1 -ing it here.
There was a problem hiding this comment.
We might do that in a way the puts the conditional into the query as well, so that we don't see this pattern repeating.
There was a problem hiding this comment.
On the idr group (see below to gauge whether this is "busy", but it looks it to me), this brute force approach takes the request from 2-2.4 seconds up to 2.2-2.5 seconds on my dev machine. The code here is taking advantage of the caching in became_rfc(), so I believe it's at least only paying the N+1 penalty and not hammering the db any worse than that.
(edit to add: the faster time is what I get if I replace all the became_rfc() calls with simple references to name or remove them entirely in the nodes.rfc logic)
We're only admitting source__type="draft", so no need to check it again in filter queries
This eliminates the attempts to get
state.slugwhenstate is None, which superficially fixes the error in ticket #6768.PR is put up as a draft because I think more attention may be needed - some of the logic identifies RFCs by using the
draftstate. I think that should look at the type instead but I'm still sorting out what's meant to be going on.Fixes #6768