Tweet
Tweet

Slide 1

Slide 1 text

REFACTORING SYSTEMS with CONFIDENCE ! # $ # %

Slide 2

Slide 2 text

NATHAN WITMER & zerowidth ' ( ) * !

Slide 3

Slide 3 text

The DOMAIN (

Slide 4

Slide 4 text

Permissions

Slide 5

Slide 5 text

Who has access to what?

Slide 6

Slide 6 text

+ Access control , Search filtering - Event feeds

Slide 7

Slide 7 text

Repository User

Slide 8

Slide 8 text

Repository User Collaborator

Slide 9

Slide 9 text

Repository Organization User Collaborator

Slide 10

Slide 10 text

Repository Organization User Collaborator Team Team Repository Team Member

Slide 11

Slide 11 text

Repository Organization User Collaborator Team Team Repository Team Member Admin Team

Slide 12

Slide 12 text

repository.pullable_by?(user)

Slide 13

Slide 13 text

user.accessible_repository_ids

Slide 14

Slide 14 text

SELECT rs.id as ris, 2 as ps from rs WHERE rs.wid = 123 UNION ALL SELECT rs.id as ris, 1 as ps from rs INNER JOIN ps ON rs.id = ps.rid WHERE ps.uid = 123 UNION ALL SELECT rs.id as ris, 2 as ps from rs INNER JOIN ts ON rs.oid = ts.oid INNER JOIN tms ON ts.id = tms.tid WHERE ts.n = 1 AND tms.uid = 123 UNION ALL SELECT rs.id as ris, GROUP_CONCAT(distinct ts.p) as ps from rs INNER JOIN tms rt ON rs.id = rt.rid INNER JOIN ts ON rt.tid = ts.id INNER JOIN tms ut ON ts.id = ut.tid WHERE ut.uid = 123 AND ts.n != 1 AND ts.p in (0,1,2) GROUP BY rs.id UNION ALL SELECT r.id as ri, 0 as p from rs JOIN us ON rs.wid = us.id JOIN ts ON ts.oid = us.id JOIN tms ON ts.id = tms.tid WHERE us.type = 0 AND ts.n = 1 AND tms.uid = 123 AND (rs.p = 0) AND rs.pid IS NOT NULL WHERE rs.id = ris

Slide 15

Slide 15 text

The PROBLEM .

Slide 16

Slide 16 text

No content

Slide 17

Slide 17 text

+ Correctness / Simplicity 0 Flexibility

Slide 18

Slide 18 text

–@rick, 2012 We could do worse than cleaning up the repo / permissioning mess, but it will take quite a bit of effort. It's part of the core domain, it's in the middle of the app, and it's going to eventually bite everyone who comes near it.

Slide 19

Slide 19 text

–@defunkt, 2012 I'd love to see someone take a step back and define all the possible relationships a user can have to a repository, then wrap that up in a tasty API.

Slide 20

Slide 20 text

The REWRITE 1

Slide 21

Slide 21 text

Abilities

Slide 22

Slide 22 text

Subject Actor action

Slide 23

Slide 23 text

Subject Actor action User Team

Slide 24

Slide 24 text

Subject Actor action User Team read write admin

Slide 25

Slide 25 text

Subject Actor action User Team Repository Team read write admin

Slide 26

Slide 26 text

+ Permissions 2 Membership

Slide 27

Slide 27 text

User Repository Team Actor Action Subject

Slide 28

Slide 28 text

User Repository Team read Actor Action Subject User read Team

Slide 29

Slide 29 text

User Repository Team read write Actor Action Subject User read Team Team write Repository

Slide 30

Slide 30 text

User Repository Team read write Actor Action Subject User read Team Team write Repository User write Repository write

Slide 31

Slide 31 text

user.can? :read, repository

Slide 32

Slide 32 text

user.can? :read, repository SELECT 1 FROM abilities WHERE actor_id = user_id AND actor_type = 'User' AND subject_id = repository_id AND subject_type = ‘Repository'

Slide 33

Slide 33 text

user.accessible_repository_ids SELECT subject_id FROM abilities WHERE actor_type = 'User' AND actor_id = user_id AND subject_type = ‘Repository'

Slide 34

Slide 34 text

The TOOLS $

Slide 35

Slide 35 text

. Does it work? 3 Is it correct? % Is it fast?

Slide 36

Slide 36 text

Legacy system New system reads

Slide 37

Slide 37 text

scientist A Ruby library for carefully refactoring critical paths

Slide 38

Slide 38 text

class Repository < ActiveRecord::Base def pullable_by?(user) # old code end end

Slide 39

Slide 39 text

class Repository < ActiveRecord::Base def pullable_by?(user) pullable_by_legacy?(user) end def pullable_by_legacy?(user) # old code end end

Slide 40

Slide 40 text

class Repository < ActiveRecord::Base def pullable_by?(user) pullable_by_legacy?(user) end def pullable_by_legacy?(user) # old code end def pullable_by_abilities?(user) user.can? :read, self end end

Slide 41

Slide 41 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) pullable_by_legacy?(user) end def pullable_by_legacy?(user) # old code end def pullable_by_abilities?(user) user.can? :read, self end end

Slide 42

Slide 42 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) # Let’s do an experiment: science "repository.pullable_by" do |experiment| end end # ... end


Slide 43

Slide 43 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) # Let’s do an experiment: science "repository.pullable_by" do |experiment| # Return this value no matter what experiment.use { pullable_by_legacy?(user) } end end # ... end

Slide 44

Slide 44 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) # Let’s do an experiment: science "repository.pullable_by" do |experiment| # Return this value no matter what experiment.use { pullable_by_legacy?(user) } # Run the new code too, and compare the results experiment.try { pullable_by_abilities?(user) } end end end

Slide 45

Slide 45 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) # Let's do an experiment science "repository.pullable_by" do |experiment| # Return this value no matter what experiment.use { pullable_by_legacy?(user) } # Run the new code too, and compare the results experiment.try { pullable_by_abilities?(user) } end end end

Slide 46

Slide 46 text

class MyExperiment < ActiveRecord::Base end

Slide 47

Slide 47 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment end

Slide 48

Slide 48 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end end

Slide 49

Slide 49 text

No content

Slide 50

Slide 50 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end def publish(result) end end

Slide 51

Slide 51 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end def publish(result) $statsd.timing "science.#{name}.control", result.control.duration $statsd.timing "science.#{name}.candidate", result.candidate.duration end end

Slide 52

Slide 52 text

No content

Slide 53

Slide 53 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end def publish(result) $statsd.timing "science.#{name}.control", result.control.duration $statsd.timing "science.#{name}.candidate", result.candidate.duration if result.matched? $statsd.increment "science.#{name}.matched" elsif result.ignored? $statsd.increment "science.#{name}.ignored" else $statsd.increment "science.#{name}.mismatched" store_mismatch_data_in_redis(result) end end end

Slide 54

Slide 54 text

No content

Slide 55

Slide 55 text

science "monster" do |experiment| experiment.class # => Scientist::Default end

Slide 56

Slide 56 text

# replace `Scientist::Default` as the implementation module Scientist::Experiment def self.new(name) MyExperiment.find_or_initialize_by(name: name) end end

Slide 57

Slide 57 text

# replace `Scientist::Default` as the implementation module Scientist::Experiment def self.new(name) MyExperiment.find_or_initialize_by(name: name) end end science "monster" do |experiment| experiment.class # => MyExperiment end

Slide 58

Slide 58 text

class MyExperiment < ActiveRecord::Base include Scientist::Experiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end def publish(result) $statsd.timing "science.#{name}.control", result.control.duration $statsd.timing "science.#{name}.candidate", result.candidate.duration if result.matched? $statsd.increment "science.#{name}.matched" elsif result.ignored? $statsd.increment "science.#{name}.ignored" else $statsd.increment "science.#{name}.mismatched" store_mismatch_data_in_redis(result) end end end

Slide 59

Slide 59 text

class MyExperiment def enabled? percent_enabled > 0 && rand(100) < percent_enabled end end

Slide 60

Slide 60 text

class MyExperiment def enabled? return true if Rails.env.test? percent_enabled > 0 && rand(100) < percent_enabled end end

Slide 61

Slide 61 text

class MyExperiment def enabled? return true if Rails.env.test? percent_enabled > 0 && rand(100) < percent_enabled end end if Rails.env.test? MyExperiment.raise_on_mismatches = true end

Slide 62

Slide 62 text

class Repository def pullable_by?(user) science "pullable_by" do |experiment| experiment.use { true } experiment.try { false } end end end

Slide 63

Slide 63 text

class Repository def pullable_by?(user) science "pullable_by" do |experiment| experiment.use { true } experiment.try { false } end end end test "repository is pullable by a collaborator" do assert repository.pullable_by?(collaborator) # => raises Scientist::Experiment::MismatchError end

Slide 64

Slide 64 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) science "repository.pullable_by" do |e| e.use { pullable_by_legacy?(user) } e.try { pullable_by_abilities?(user) } end end end

Slide 65

Slide 65 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) science "repository.pullable_by" do |e| e.context :user => user.id, :repository => id e.use { pullable_by_legacy?(user) } e.try { pullable_by_abilities?(user) } end end end

Slide 66

Slide 66 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) science "repository.pullable_by" do |e| e.context :user => user.id, :repository => id e.use { pullable_by_legacy?(user) } e.try { pullable_by_abilities?(user) } e.ignore { user.staff? } end end end

Slide 67

Slide 67 text

class Repository < ActiveRecord::Base include Scientist def pullable_by?(user) science "repository.pullable_by" do |e| e.context :user => user.id, :repository => id e.use { pullable_by_legacy?(user) } e.try { pullable_by_abilities?(user) } e.ignore { user.staff? } e.ignore do |control, candidate| # unconfirmed users aren't handled yet: control && !candidate && user.unconfirmed? end end end end

Slide 68

Slide 68 text

class User def teams science "user.teams" do |e| e.use { legacy_teams } # DatabaseTeams e.try { new_teams } # WebServiceTeams end end end

Slide 69

Slide 69 text

class User def teams science "user.teams" do |e| e.use { legacy_teams } # DatabaseTeams e.try { new_teams } # WebServiceTeams e.compare do |control, candidate| control == candidate end end end end

Slide 70

Slide 70 text

class User def teams science "user.teams" do |e| e.use { legacy_teams } # DatabaseTeams e.try { new_teams } # WebServiceTeams e.compare do |control, candidate| control.map(&:id) == candidate.map(&:id) end end end end

Slide 71

Slide 71 text

. Does it work? 3 Is it correct? % Is it fast?

Slide 72

Slide 72 text

Legacy system New system reads

Slide 73

Slide 73 text

Legacy system New system reads writes

Slide 74

Slide 74 text

class Team def add_member(user) legacy_members << user # old system end end

Slide 75

Slide 75 text

class Team def add_member(user) legacy_members << user # old system grant user, :read # new system end end

Slide 76

Slide 76 text

class Team def add_member(user) legacy_members << user # old system grant user, :read # new system end def grant(actor, permission) return unless Abilities.enabled? # ... end end

Slide 77

Slide 77 text

Legacy system New system reads writes

Slide 78

Slide 78 text

Legacy system New system reads writes validate and backfill

Slide 79

Slide 79 text

The LESSONS 4

Slide 80

Slide 80 text

Documentation

Slide 81

Slide 81 text

Refactoring for understanding

Slide 82

Slide 82 text

Refactoring for simplicity

Slide 83

Slide 83 text

class Team has_many :memberships has_many :users, :through => :memberships end class Membership belongs_to :team belongs_to :user end class User has_many :memberships has_many :teams, :through => :memberships end

Slide 84

Slide 84 text

# Team <--> Membership <--> User Membership.create! team.users << user user.teams << team membership.destroy team.users.delete user user.teams.delete team

Slide 85

Slide 85 text

team.add_member(user) team.remove_member(user)

Slide 86

Slide 86 text

Production data is the real test

Slide 87

Slide 87 text

Math is important

Slide 88

Slide 88 text

User Repository Team

Slide 89

Slide 89 text

User Repository Team One team, with 10,000 users and 5,000 repositories:

Slide 90

Slide 90 text

User Repository Team read One team, with 10,000 users and 5,000 repositories: 10,000 user → team records

Slide 91

Slide 91 text

User Repository Team read write One team, with 10,000 users and 5,000 repositories: 10,000 user → team records 5,000 team → repository records

Slide 92

Slide 92 text

User Repository Team read write write One team, with 10,000 users and 5,000 repositories: 10,000 user → team records 5,000 team → repository records 50,000,000 user → repository records

Slide 93

Slide 93 text

User Repository Team read write write One team, with 10,000 users and 5,000 repositories: 10,000 user → team records 5,000 team → repository records 50,000,000 user → repository records

Slide 94

Slide 94 text

Measurements give you confidence

Slide 95

Slide 95 text

No content

Slide 96

Slide 96 text

Scientist for everything?

Slide 97

Slide 97 text

class User def search_repositories(query) science "new-search-cluster" do e.use { search_old_cluster(query) } e.try { search_new_cluster(query) } e.compare { true } # don't care end end end

Slide 98

Slide 98 text

Incremental development

Slide 99

Slide 99 text

The RESULT %

Slide 100

Slide 100 text

Abilities system is authoritative

Slide 101

Slide 101 text

The code is cleaner

Slide 102

Slide 102 text

+ Correctness / Simplicity 0 Flexibility

Slide 103

Slide 103 text

?

Slide 104

Slide 104 text

THANKS! ♥ & github/scientist