Instead of returning a blank result it throws exceptions. Fix this up a
little so we get some consistency with differential
Details
Details
- Reviewers
epriestley dschleimer • elenaperezrioja - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rP4d1709651e4e: [releeph] Conduit failure with bad IDs
Loaded a bad phid for releeph, returns empty list.
Try a good phid and get 2 releeph merges.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
The more consistent approach is to not return results which don't match parameters. differential.query, which is the parallel to this method, works the same way, as do all other modern query methods.
In particular, if you have 30 PHIDs and one of them is bad, "return nothing for nonmatching results" lets you handle that reasonably, while "throw an exception and return no results at all" doesn't.
Comment Actions
This will return the 29/30 correctly, I only set the bad flag when all are bad. Without it you end up with a full table scan since there is no where filtering applied.
Comment Actions
Ohhhh, sorry! I was misunderstanding what's going on here.
The most standard way to approach this would be to remove the logic from withRevisionPHIDs() and just store $this->revisionPHIDs = ..., then do the subquery and Empty check inside buildWhereClause(). This would remove the need for the badRevisionsRequested flag.
For most types of subquery logic, the check must be performed in this way because the subqueries require a viewer. This logic will eventually need to be adjusted to account for that, because it skips policy checks on the revisions right now. Once policies roll out fully, you should not be allowed to query requests for a revision you don't have permission to see. This is fine for the moment, though, as we're kind of ignoring releeph for policy purposes for the moment and it has a bunch of other stuff that needs cleanup.