Summary

Generate WebAPI resource paths with backtracking

Review Request #10117 — Created Aug. 13, 2018 and submitted Aug. 23, 2018, 2:23 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10112~~

Blocks

10118

Commit

a508824...

Reviewers

Groups

reviewboard

People

Description

The old method of generating fake resource paths assumed that the first
model instance that matched resource.get_queryset(...) would be
suitable in all cases, i.e., this object would have all optional
relationships that corresspond to models. For example, it assumed that
the last modified review request would have all of the following:
- A review request draft
- File attachments
- Screenshots
- Reviews
- Draft file attachments
- Draft screenshots
- Draft reviews
- Review replies
- etc.

This is problematic when we want to add a new model instance (e.g., for
the in-process DVCs work which adds new models) because all these child
objects will have to be created for the last modified model instance.

Now, we use a backtracking algorithm to iterate over path-model pairs so
that if a model does not have the requisite child objects we can
continue onto the next instance. This way, we only need to add objects
to the database that correspond to new resources, instead of duplicating
objects from the previous parent object or recreating them entirely.

Testing Done

Built the docs.
With this patch applied, the docs in /r/10118/ build correctly.

Issues

Description	From	Last Updated
Typo in the description: "fakes resource paths" -> "fake resource paths" Also in the second paragraph: "the in process DVCs …	chipx86	Aug. 23, 2018, 2:16 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Aug. 13, 2018, 4:06 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 13, 2018, 4:05 p.m.
F841 local variable 'inner_exc' is assigned to but never used	reviewbot	Aug. 13, 2018, 4:05 p.m.
F841 local variable 'e' is assigned to but never used	reviewbot	Aug. 13, 2018, 5:03 p.m.
Needs to be the full namespace.	chipx86	Aug. 17, 2018, 6:43 p.m.
Blank line between these.	chipx86	Aug. 17, 2018, 6:43 p.m.
Why are we looping and then returning in the first iteration?	chipx86	Aug. 17, 2018, 6:46 p.m.
The wording seems pretty off: "Could not path for ...". I think this is missing a word.	chipx86	Aug. 17, 2018, 6:46 p.m.
Can you add an assertion error message explaining what the comment explained? That way if it's actually hit, we know …	chipx86	Aug. 17, 2018, 6:47 p.m.
The not (not ..) is kinda weird. Can we rewrite to inverse the clauses, so we have: if (not resource.singleton …	chipx86	Aug. 17, 2018, 6:50 p.m.
Mind adding a comment saying why we might be hitting this from the yield? It'll help with making this more …	chipx86	Aug. 17, 2018, 6:58 p.m.
Same comment as above.	chipx86	Aug. 17, 2018, 6:58 p.m.
bool	chipx86	Aug. 23, 2018, 2:20 p.m.
bool	chipx86	Aug. 23, 2018, 2:20 p.m.

flake8 failed.

JSHint passed.

flake8

docs/manual/_ext/webapidocs.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
docs/manual/_ext/webapidocs.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (82 > 79 characters)
```
docs/manual/_ext/webapidocs.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'inner_exc' is assigned to but never used
```

Depends On:

+	~~10112 - Update docs fixtures with DVCS model changes~~

Commit:

-	93785c0f495f0d74c47c9c98b2bc95f01246ddab
+	58cbabbb51769f1378bd0c806bf5a27cfcb74e25

Diff:

Revision 2 (+100 -25)

Show changes

docs/manual/_ext/webapidocs.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

docs/manual/_ext/webapidocs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'e' is assigned to but never used
```

Commit:

-	58cbabbb51769f1378bd0c806bf5a27cfcb74e25
+	236e1ff759e02c6ab3993eb529918785bd9ad8d2

Diff:

Revision 3 (+100 -25)

Show changes

docs/manual/_ext/webapidocs.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Typo in the description: "fakes resource paths" -> "fake resource paths"

Also in the second paragraph: "the in process DVCs work" "the in-progress DVCS work"

chipx86 Aug. 23, 2018, 1:15 p.m.

Still has "DVCs". Should be "DVCS".

Also, "in-process" should be "in-progress," right?

docs/manual/_ext/webapidocs.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Needs to be the full namespace.
```
docs/manual/_ext/webapidocs.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```
docs/manual/_ext/webapidocs.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Why are we looping and then returning in the first iteration?
```

docs/manual/_ext/webapidocs.py (Diff revision 3)

The issue has been resolved. Show all issues

The wording seems pretty off: "Could not path for ...". I think this is missing a word.

docs/manual/_ext/webapidocs.py (Diff revision 3)

The issue has been resolved. Show all issues

Can you add an assertion error message explaining what the comment explained? That way if it's actually hit, we know why instead of having to dig into code.

docs/manual/_ext/webapidocs.py (Diff revision 3)

The issue has been resolved. Show all issues

The not (not ..) is kinda weird. Can we rewrite to inverse the clauses, so we have:

if (not resource.singleton and ...):
    q = resource.get_queryset(...)
    ...
else:
    try:
        yield ...
    except ...

docs/manual/_ext/webapidocs.py (Diff revision 3)

The issue has been resolved. Show all issues

Mind adding a comment saying why we might be hitting this from the yield? It'll help with making this more maintainable (since it is a more complicated set of logic).

brennie Aug. 17, 2018, 6:55 p.m.

I apparently had a brain fart while I was writing this. This code does nothing.

docs/manual/_ext/webapidocs.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same comment as above.
```

Change Summary:

Addressed Christian's issues

Description:

~		The old method of generating fakes resource paths assumed that the first
	~	The old method of generating fake resource paths assumed that the first
		model instance that matched `resource.get_queryset(...)` would be
		suitable in all cases, i.e., this object would have all optional
		relationships that corresspond to models. For example, it assumed that
		the last modified review request would have all of the following:
		- A review request draft
		- File attachments
		- Screenshots
		- Reviews
		- Draft file attachments
		- Draft screenshots
		- Draft reviews
		- Review replies
		- etc.

		This is problematic when we want to add a new model instance (e.g., for
~		the in process DVCs work which adds new models) because all these child
	~	the in-process DVCs work which adds new models) because all these child
		objects will have to be created for the last modified model instance.

		Now, we use a backtracking algorithm to iterate over path-model pairs so
		that if a model does not have the requisite child objects we can
		continue onto the next instance. This way, we only need to add objects
		to the database that correspond to new resources, instead of duplicating
		objects from the previous parent object or recreating them entirely.

Commit:

-	236e1ff759e02c6ab3993eb529918785bd9ad8d2
+	a508824e55213b77f743d3de3a3f3f3b3d6983a2

Diff:

Revision 4 (+98 -24)

Show changes

docs/manual/_ext/webapidocs.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

docs/manual/_ext/webapidocs.py (Diff revision 4)
The issue has been resolved. Show all issues
```
bool
```
docs/manual/_ext/webapidocs.py (Diff revision 4)
The issue has been resolved. Show all issues
```
bool
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (1625f5c)