Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Branch protection: Possibility to not use whitelist but allow anyone with write access #9055

Merged
merged 28 commits into from
Dec 4, 2019
Merged

Branch protection: Possibility to not use whitelist but allow anyone with write access #9055

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
22be3e9
Possibility to not use whitelist but allow anyone with write access
davidsvantesson Nov 17, 2019
11a5954
fix existing test
davidsvantesson Nov 17, 2019
5bcf80e
rename migration function
davidsvantesson Nov 17, 2019
b488f1d
Try to give a better name for migration step
davidsvantesson Nov 17, 2019
21f8590
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 17, 2019
2461d52
Merge remote-tracking branch 'upstream/master' into branch-protection…
davidsvantesson Nov 18, 2019
d1ecef6
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 18, 2019
d48fcb4
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 21, 2019
459c59c
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 22, 2019
283be88
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 23, 2019
9d3da22
Clear settings if higher level setting is not set
davidsvantesson Nov 23, 2019
148e1d6
Move official reviews to db instead of counting approvals each time
davidsvantesson Nov 25, 2019
8afcf90
migration
davidsvantesson Nov 25, 2019
c296126
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 25, 2019
5ee7ae2
fix
davidsvantesson Nov 25, 2019
b63974e
fix migration
davidsvantesson Nov 26, 2019
6b04006
fix migration
davidsvantesson Nov 26, 2019
e6908f5
Remove NOT NULL from EnableWhitelist as migration isn't possible
davidsvantesson Nov 26, 2019
2cad7bb
Fix migration, reviews are connected to issues.
davidsvantesson Nov 26, 2019
8af34e3
Fix SQL query issues in GetReviewersByPullID.
davidsvantesson Nov 26, 2019
2187c6f
Merge branch 'master' into branch-protection-anyone
davidsvantesson Nov 26, 2019
b1537d4
Simplify function GetReviewersByIssueID
davidsvantesson Nov 27, 2019
8198531
Handle reviewers that has been deleted
davidsvantesson Nov 27, 2019
5aa2e18
Ensure reviews for test is in a well defined order
davidsvantesson Nov 27, 2019
6ca746a
Only clear and set official reviews when it is an approve or reject.
davidsvantesson Nov 28, 2019
f81f209
Merge branch 'master' into branch-protection-anyone
techknowlogick Dec 1, 2019
52760dd
Merge branch 'master' into branch-protection-anyone
davidsvantesson Dec 3, 2019
93ede5a
Merge branch 'master' into branch-protection-anyone
techknowlogick Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions models/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,9 @@ func GetReviewersByPullID(pullID int64) (issueReviewers []*PullReviewersWithType
irs := []*PullReviewersWithType{}
if x.Dialect().DBType() == core.MSSQL {
err = x.SQL(`SELECT [user].*, review.type, review.official, review.review_updated_unix FROM
(SELECT review.id, review.type, review.reviewer_id, max(review.updated_unix) as review_updated_unix
(SELECT review.id, review.type, review.reviewer_id, review.official, max(review.updated_unix) as review_updated_unix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line I don't understand. If you have review.id in the row, all lines are already unique (unless there were multiple users with the same id which should never happen). A MAX(updated_unix) will be the same as updated_unix without the Group By. 🤔

What is it that you are trying to accomplish? Bring up the latest review? The query from the migration should achieve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function shall return the latest review of each reviewer (not necessary official reviewers). I see that I actually don't use the change in this function, probably rewrote something. I think later on the information on which reviews are "official" should be shown on the pull request page.

The max(review.updated_unix) as review_updated_unix is used to sort the list and ensure the group by gets the latest review. However it might have been enough to get the max(id) instead, I don't see how reviews from same reviewer could change order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function shall return the latest review of each reviewer (not necessary official reviewers). I see that I actually don't use the change in this function, probably rewrote something. I think later on the information on which reviews are "official" should be shown on the pull request page.

The max(review.updated_unix) as review_updated_unix is used to sort the list and ensure the group by gets the latest review. However it might have been enough to get the max(id) instead, I don't see how reviews from same reviewer could change order.

This kind of query will not work. If you have two different review.type (e.g. comment + approval) from the same user, you'll get two rows because Group By will see them as different values. To get the latest record you need to do a select in select like the one used in the migration:

if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?) GROUP BY issue_id, reviewer_id)",
			issueID, models.ReviewTypeApprove, models.ReviewTypeReject).
			Find(&reviews); err != nil {
			return err
		}

Of course the example above requires some adaptation to get the JOIN and the columns you're expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually solved further down in the function (I don't say it is a good solution 😄)

	// We need to group our results by the fields we want, otherwise some databases will fail (only_full_group_by).
	// But becaus we're doing this, we need to manually filter out multiple reviews of different types by the
	// same person because we only want to show the newest review grouped by user. Thats why we're using a map here.

Copy link
Contributor Author

@davidsvantesson davidsvantesson Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your query might work... although it doesn't work to just remove some of the group by fields for pgsql and mysql (due to some only_full_group_by setting).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I didn't want to touch this function more than needed (and actually I can revert the changes I done). But the only thing I added was the Official field to the results, and hence added in SQL where necessary. The multiple rows is handled by the code later already.

I think maybe this function tries to do too much in SQL which makes it a bit complex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To exemplify this, let me give you one example. Suppose you have this in the review table:

review ID user_id review_type review_date
1 1 comment Jan 1
2 2 request-changes Jan 2
3 1 comment Jan 3
4 3 comment Jan 4
5 2 approval Jan 5

If you do:

select max(review_id), user_id, review_type, max(review_date)
from review
group by user_id, review_type;

This is what you will get:

review ID user_id review_type review_date
2 2 request-changes Jan 2
3 1 comment Jan 3
4 3 comment Jan 4
5 2 approval Jan 5

As you see, the two reviews from user #1 were consolidated in one row, as the fields user_id and review_type are the same for all records; user #3 is not a problem, but the two reviews from user #2 show up as different rows.

That's why the only way to get this SQL right is with a sub-query:

select max(id), user_id from review group by user_id;

Will give you:

review ID user_id
3 1
4 3
5 2

So, these are the records that you're looking for:

select review_id, user_id, review_type, review_date
from `review`
where review_id in (
        select max(id), user_id from `review` as `review_inner` group by user_id
    )

returns:

review ID user_id review_type date
3 1 comment Jan 3
4 3 comment Jan 4
5 2 approval Jan 5

which is what you want. With this method you can perform joins too:

select review_id, user_id, review_type, review_date, `user`.*
from `review`
INNER JOIN `user` ON `user`.`id` = `review`.`user_id`
where review_id in (
        select max(id), user_id from `review` as `review_inner` group by user_id
    )

There's probably a nice way to write all of this with a couple of xorm directives (e.g. I think you can write the subquery in builder and use it in the In() operator, but I'm not sure), so I think it will be just quicker to do it with sess.SQL() like you did in migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very long informative response 😄.
I just wanted to point out that the existing code actually works, although not very efficient or elegant. I will see if I can rewrite the SQL query in a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry! I didn't realize that the code wasn't yours. It's my tendency of checking the right column more thoroughly than the left one. 😋

If you just don't feel like doing that change just let me know, because that's the only thing I was waiting for to give my 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote the code and think it is easier to understand now although there will be a few more sql queries, but normally there will not be that many reviews for a PR.

FROM review WHERE review.issue_id=? AND (review.type = ? OR review.type = ?)
GROUP BY review.id, review.type, review.reviewer_id) as review
GROUP BY review.id, review.type, review.reviewer_id, review.official) as review
INNER JOIN [user] ON review.reviewer_id = [user].id ORDER BY review_updated_unix DESC`,
pullID, ReviewTypeApprove, ReviewTypeReject).
Find(&irs)
Expand All @@ -357,12 +357,12 @@ INNER JOIN [user] ON review.reviewer_id = [user].id ORDER BY review_updated_unix
Join("INNER", "`user`", "review.reviewer_id = `user`.id").
Where("review.issue_id = ? AND (review.type = ? OR review.type = ?)",
pullID, ReviewTypeApprove, ReviewTypeReject).
GroupBy("`user`.id, review.type").
GroupBy("`user`.id, review.type, review.official").
OrderBy("review_updated_unix DESC").
Find(&irs)
}

// We need to group our results by user id _and_ review type, otherwise the query fails when using postgresql.
// We need to group our results by the fields we want, otherwise some databases will fail (only_full_group_by).
// But becaus we're doing this, we need to manually filter out multiple reviews of different types by the
// same person because we only want to show the newest review grouped by user. Thats why we're using a map here.
issueReviewers = []*PullReviewersWithType{}
Expand Down