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.

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

[api][webui] Add polymorphic association to review #3070

Merged
merged 1 commit into from May 5, 2017

Conversation

Projects
None yet
6 participants
@ChrisBr
Copy link
Member

ChrisBr commented May 4, 2017

entities are user, group, project and package.
This makes ActiveModel queries more simple and efficient.
Also introduced validation that setting only one entity is allowed.
Furthermore changed the collation of the by_ fields to utf8_general_ci
as joining on these fields was not possible (e.g. group#title had
a mismatched collation).
This was already done on build.o.o manually in the past so
this commit will make the structures more consistent.

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 4, 2017

Codecov Report

Merging #3070 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3070      +/-   ##
==========================================
+ Coverage   88.71%   88.75%   +0.04%     
==========================================
  Files         258      258              
  Lines       17521    17541      +20     
==========================================
+ Hits        15544    15569      +25     
+ Misses       1977     1972       -5
Flag Coverage Δ
#api 84.44% <95.83%> (+0.8%) ⬆️
#rspec 64.86% <100%> (+0.08%) ⬆️
#webui 63.91% <95.83%> (-0.27%) ⬇️
@evanrolfe
Copy link

evanrolfe left a comment

Christian can you confirm how long the migration takes to run against production data? Thanks

@@ -39,6 +40,8 @@ class NotFoundError < APIException
end
end

before_validation :set_entity_association

This comment has been minimized.

@evanrolfe

evanrolfe May 4, 2017

Doesn't this mean that the entity will only be set if you call review.valid?? What happens if you dont call .valid? like here?

review = Review.new(by_user: user)
review.save

I think this should be on before_save instead?

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

Nope, the validation also runs for save etc. It basically runs always except you skip the validation or on some rare operations (e.g. touch). We even have a Rubocop rule that we don't skip model validations:
https://github.com/openSUSE/open-build-service/blob/master/.rubocop.yml#L265

I do it in before_validation so that I can rely on it in the validation, e.g.
if by_user && !entity
Otherwise I would need to query for the data twice (once in the validation and another time in the before_save to set the validation).

See also the ActiveRecordCallbacks here http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html

context 'with valid attributes' do
it 'sets user association when by_user object exists' do
review = Review.create(by_user: user.login)
expect(review.valid?).to eq(true)

This comment has been minimized.

@evanrolfe

evanrolfe May 4, 2017

We shouldn't have to explicitly call .valid? in order to have the entity attribute populated.

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

The valid is not necessary to populate the entity, this already happens with the create.

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

I dropped the valid!

@@ -94,23 +97,31 @@ def check_initial
errors.add(:unknown, 'no reviewer defined')
end

if by_user && !User.find_by_login(by_user)
if by_user

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

Those 5 lines can be just one "big" condition in the line 106, something like:

errors.add(:base,.......) if (by_user && (by_group || by_project || by_package)) || 
                             (by_group && (by_project || by_package))

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

Do you think this is more readable?
If you prefer I can do it but actually I think this is more complicated and harder to read!

This comment has been minimized.

@bgeuken

bgeuken May 4, 2017

Member

I agree with Christian. This wouldn't be very readable

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

Ok, no worries

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

So, assigning a variable (not used anymore) to true or nil (because is not set before) by having a flow control structure of 2 levels of conditions is more clear than just a composited condition? I don't see it... I can understand the "complex" condition without any problem... the other is just hurting my eyes 😈

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

More readable... and also that "complex" logic is better hidden

This comment has been minimized.

@evanrolfe

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

I don't know if it worth to be a private method... and also if the name is the proper one

This comment has been minimized.

@bgeuken

bgeuken May 4, 2017

Member

This surely can be written in various ways, eg. I would have preferred to have that specific validation in a separate validation method. Though I didn't find it important enough to require this for in PR.

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

I will change this as it will increase the readability. Let's talk about moving this to a dedicated Validator class in another PR.


errors.add(:base, 'it is not allowed to have more than one reviewer entity: by_user, by_group, by_project, by_package.') if error

if by_user && !entity

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

Use guard clause style:

errors.add(:by_user, "#{by_user} not found") if by_user && !entity

This comment has been minimized.

@ChrisBr
errors.add(:by_user, "#{by_user} not found")
end

if by_group && !Group.find_by_title(by_group)
if by_group && !entity

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

I know that you only changed on thing here... but better if we use guard clause style

This comment has been minimized.

@ChrisBr
errors.add(:by_group, "#{by_group} not found")
end

if by_project && !Project.find_by_name(by_project)
if by_project && !entity

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

I know that you only changed on thing here... but better if we use guard clause style

This comment has been minimized.

@ChrisBr
errors.add(:by_group, "#{by_group} not found")
end

if by_project && !Project.find_by_name(by_project)
if by_project && !entity
# must be a local project or we can't ask
errors.add(:by_project, "#{by_project} not found")
end

if by_package && !by_project

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

better if we use guard clause style BTW

This comment has been minimized.

@bgeuken

bgeuken May 4, 2017

Member

Why is it better?

This comment has been minimized.

@evanrolfe

evanrolfe May 4, 2017

Yeah I prefer the way its already coded.

This comment has been minimized.

@mdeniz

This comment has been minimized.

@bgeuken

bgeuken May 4, 2017

Member

So it's better because someone in the internet prefers it?:D

To me there are cases where a guard clause is nicer and other cases where the other format is nicer. Here I don't see why it has to be in the guard clause format.

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

So as there is no clear conclusion and it is anyway unrelated to this PR I will leave it as it is!

This comment has been minimized.

@mdeniz

mdeniz May 5, 2017

Contributor

"So it's better because someone in the internet prefers it?:D" ... Did I said that I prefer it? Yes, I did it 😈

For me also the point in this case is.. we have 2 guard clause style conditions and 3 in the other style.... at least I prefer to have them in the same style 😸


def down
remove_reference(:reviews, :entity, polymorphic: true)
end

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

What about reverting the ALTER TABLE for the collations?

This comment has been minimized.

@ChrisBr

ChrisBr May 4, 2017

Member

Hmm, right!

describe 'validations' do
it 'is not allowed to specify by_user and any other entity' do
[:by_group, :by_project, :by_package].each do |entity|
review = Review.new

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

What about this instead of lines 36 to 39 ?:

review = Review.create(:by_user => user.login, entity => 'not-existent-entity')

This comment has been minimized.

@ChrisBr

it 'is not allowed to specify by_group and any other entity' do
[:by_project, :by_package].each do |entity|
review = Review.new

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

Same here

This comment has been minimized.

@ChrisBr
context 'with valid attributes' do
it 'sets user association when by_user object exists' do
review = Review.create(by_user: user.login)
expect(review.valid?).to eq(true)

This comment has been minimized.

@mdeniz

mdeniz May 4, 2017

Contributor

I usually prefer to have only one expectation per it block.
BTW, do we have Review a factory, right?

This comment has been minimized.

@evanrolfe

evanrolfe May 4, 2017

I dont think that a factory would be useful here. This is about testing that when you call Review.create it populates the entity columns so IMO its better to call Review.create explicitely.

@@ -22,6 +22,7 @@ class NotFoundError < APIException
validate :validate_non_symmetric_assignment
validate :validate_not_self_assigned

belongs_to :entity, polymorphic: true

This comment has been minimized.

@hennevogel

hennevogel May 4, 2017

Member

:entity is too generic for my taste. :reviewable?

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 4, 2017

== 20170426153510 AddReferencesToReview: migrating ============================
-- execute("ALTER TABLE reviews MODIFY by_user VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_general_ci;")
   -> 28.9161s
-- execute("ALTER TABLE reviews MODIFY by_group VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_general_ci;")
   -> 29.5294s
-- execute("ALTER TABLE reviews MODIFY by_project VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_general_ci;")
   -> 27.6763s
-- execute("ALTER TABLE reviews MODIFY by_package VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_general_ci;")
   -> 27.7294s
-- add_reference(:reviews, :entity, {:polymorphic=>true})
   -> 130.1108s
-- execute("      UPDATE reviews\n      INNER JOIN users ON users.login = reviews.by_user\n      SET reviews.entity_id = users.id, reviews.entity_type = 'User'\n      WHERE reviews.by_user IS NOT NULL\n")
   -> 10.6599s
-- execute("      UPDATE reviews\n      INNER JOIN groups ON groups.title = reviews.by_group\n      SET reviews.entity_id = groups.id, reviews.entity_type = 'Group'\n      WHERE reviews.by_group IS NOT NULL\n")
   -> 43.8764s
-- execute("      UPDATE reviews\n      INNER JOIN projects ON projects.name = reviews.by_project\n      SET reviews.entity_id = projects.id, reviews.entity_type = 'Project'\n      WHERE reviews.by_project IS NOT NULL AND reviews.by_package IS NULL\n")
   -> 13.9456s
-- execute("      UPDATE reviews\n      INNER JOIN projects ON projects.name = reviews.by_project\n      INNER JOIN packages ON packages.name = reviews.by_package AND packages.project_id = projects.id\n      SET reviews.entity_id = packages.id, reviews.entity_type = 'Package'\n      WHERE reviews.by_project IS NOT NULL AND reviews.by_package IS NOT NULL\n")
   -> 3.4278s
== 20170426153510 AddReferencesToReview: migrated (315.8734s) =================

~5 minutes

However, note that on build.o.o the by_ fields have already utf8_unicode_ci collation, so it should be faster.

@ChrisBr ChrisBr force-pushed the ChrisBr:associations_review branch 3 times, most recently from 85fcdf1 to 8cd2a06 May 4, 2017

@evanrolfe

This comment has been minimized.

Copy link

evanrolfe commented May 4, 2017

LGTM

[api][webui] Add polymorphic association to review
entities are user, group, project and package.
This makes ActiveModel queries more simple and efficient.
Also introduced validation that setting only one entity is allowed.
Furthermore changed the collation of the by_ fields to utf8_general_ci
as joining on these fields was not possible (e.g. group#title had
a mismatched collation).
This was already done on build.o.o manually in the past so
this commit will make the structures more consistent.

@ChrisBr ChrisBr force-pushed the ChrisBr:associations_review branch from 8cd2a06 to c03bc99 May 4, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 5, 2017

@mdeniz please review and approve!

@mdeniz

mdeniz approved these changes May 5, 2017

@mdeniz mdeniz merged commit e1fd3d2 into openSUSE:master May 5, 2017

4 checks passed

Hakiri No security warnings were found.
Details
codecov/patch 100% of diff hit (target 88.71%)
Details
codecov/project 88.75% (+0.04%) compared to 0282179
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ChrisBr ChrisBr deleted the ChrisBr:associations_review branch Jul 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment