Skip to content

Commit b01054d

Browse files
byrootbyroot
authored
Merge pull request rails#42776 from Shopify/refactor-type-map
Refactor ActiveRecord::Type::TypeMap
2 parents 1bfa963 + cf7db80 commit b01054d

File tree

3 files changed

+146
-101
lines changed

3 files changed

+146
-101
lines changed

‎activerecord/lib/active_record/type/hash_lookup_type_map.rb

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,40 @@
22

33
module ActiveRecord
44
module Type
5-
class HashLookupTypeMap < TypeMap # :nodoc:
5+
class HashLookupTypeMap # :nodoc:
6+
def initialize(parent = nil)
7+
@mapping = {}
8+
@cache = Concurrent::Map.new do |h, key|
9+
h.fetch_or_store(key, Concurrent::Map.new)
10+
end
11+
end
12+
13+
def lookup(lookup_key, *args)
14+
fetch(lookup_key, *args) { Type.default_value }
15+
end
16+
17+
def fetch(lookup_key, *args, &block)
18+
@cache[lookup_key].fetch_or_store(args) do
19+
perform_fetch(lookup_key, *args, &block)
20+
end
21+
end
22+
23+
def register_type(key, value = nil, &block)
24+
raise ::ArgumentError unless value || block
25+
26+
if block
27+
@mapping[key] = block
28+
else
29+
@mapping[key] = proc { value }
30+
end
31+
@cache.clear
32+
end
33+
34+
def clear
35+
@mapping.clear
36+
@cache.clear
37+
end
38+
639
def alias_type(type, alias_type)
740
register_type(type) { |_, *args| lookup(alias_type, *args) }
841
end

‎activerecord/lib/active_record/type/type_map.rb

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,55 +8,49 @@ class TypeMap # :nodoc:
88
def initialize(parent = nil)
99
@mapping = {}
1010
@parent = parent
11-
@cache = Concurrent::Map.new do |h, key|
12-
h.fetch_or_store(key, Concurrent::Map.new)
13-
end
11+
@cache = Concurrent::Map.new
1412
end
1513

16-
def lookup(lookup_key, *args)
17-
fetch(lookup_key, *args) { Type.default_value }
14+
def lookup(lookup_key)
15+
fetch(lookup_key) { Type.default_value }
1816
end
1917

20-
def fetch(lookup_key, *args, &block)
21-
@cache[lookup_key].fetch_or_store(args) do
22-
perform_fetch(lookup_key, *args, &block)
18+
def fetch(lookup_key, &block)
19+
@cache.fetch_or_store(lookup_key) do
20+
perform_fetch(lookup_key, &block)
2321
end
2422
end
2523

2624
def register_type(key, value = nil, &block)
2725
raise ::ArgumentError unless value || block
28-
@cache.clear
2926

3027
if block
3128
@mapping[key] = block
3229
else
3330
@mapping[key] = proc { value }
3431
end
32+
@cache.clear
3533
end
3634

3735
def alias_type(key, target_key)
38-
register_type(key) do |sql_type, *args|
36+
register_type(key) do |sql_type|
3937
metadata = sql_type[/\(.*\)/, 0]
40-
lookup("#{target_key}#{metadata}", *args)
38+
lookup("#{target_key}#{metadata}")
4139
end
4240
end
4341

44-
def clear
45-
@mapping.clear
46-
end
47-
4842
protected
49-
def perform_fetch(lookup_key, *args, &block)
43+
def perform_fetch(lookup_key)
5044
matching_pair = @mapping.reverse_each.detect do |key, _|
5145
key === lookup_key
5246
end
5347

5448
if matching_pair
55-
matching_pair.last.call(lookup_key, *args)
49+
matching_pair.last.call(lookup_key)
5650
elsif @parent
57-
@parent.perform_fetch(lookup_key, *args, &block)
51+
@parent.perform_fetch(lookup_key)
5852
else
59-
yield lookup_key, *args
53+
yield lookup_key
6054
end
6155
end
6256
end

‎activerecord/test/cases/type/type_map_test.rb

Lines changed: 99 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,60 @@
44

55
module ActiveRecord
66
module Type
7-
class TypeMapTest < ActiveRecord::TestCase
7+
module TypeMapSharedTests
88
def test_default_type
9-
mapping = TypeMap.new
9+
mapping = klass.new
1010

1111
assert_kind_of Value, mapping.lookup(:undefined)
1212
end
1313

14+
def test_requires_value_or_block
15+
mapping = klass.new
16+
17+
assert_raises(ArgumentError) do
18+
mapping.register_type(/only key/i)
19+
end
20+
end
21+
22+
def test_fetch
23+
mapping = klass.new
24+
mapping.register_type(1, "string")
25+
26+
assert_equal "string", mapping.fetch(1) { "int" }
27+
assert_equal "int", mapping.fetch(2) { "int" }
28+
end
29+
30+
def test_fetch_memoizes
31+
mapping = klass.new
32+
33+
looked_up = false
34+
mapping.register_type(1) do
35+
fail if looked_up
36+
looked_up = true
37+
"string"
38+
end
39+
40+
assert_equal "string", mapping.fetch(1)
41+
assert_equal "string", mapping.fetch(1)
42+
end
43+
44+
def test_register_clears_cache
45+
mapping = klass.new
46+
47+
mapping.register_type(1, "string")
48+
mapping.lookup(1)
49+
mapping.register_type(1, "int")
50+
51+
assert_equal "int", mapping.lookup(1)
52+
end
53+
end
54+
55+
class TypeMapTest < ActiveRecord::TestCase
56+
include TypeMapSharedTests
57+
1458
def test_registering_types
1559
boolean = Boolean.new
16-
mapping = TypeMap.new
60+
mapping = klass.new
1761

1862
mapping.register_type(/boolean/i, boolean)
1963

@@ -23,26 +67,17 @@ def test_registering_types
2367
def test_overriding_registered_types
2468
time = Time.new
2569
timestamp = DateTime.new
26-
mapping = TypeMap.new
70+
mapping = klass.new
2771

2872
mapping.register_type(/time/i, time)
2973
mapping.register_type(/time/i, timestamp)
3074

3175
assert_equal mapping.lookup("time"), timestamp
3276
end
3377

34-
def test_fuzzy_lookup
35-
string = +""
36-
mapping = TypeMap.new
37-
38-
mapping.register_type(/varchar/i, string)
39-
40-
assert_equal mapping.lookup("varchar(20)"), string
41-
end
42-
4378
def test_aliasing_types
4479
string = +""
45-
mapping = TypeMap.new
80+
mapping = klass.new
4681

4782
mapping.register_type(/string/i, string)
4883
mapping.alias_type(/varchar/i, "string")
@@ -53,17 +88,17 @@ def test_aliasing_types
5388
def test_changing_type_changes_aliases
5489
time = Time.new
5590
timestamp = DateTime.new
56-
mapping = TypeMap.new
91+
mapping = klass.new
5792

5893
mapping.register_type(/timestamp/i, time)
5994
mapping.alias_type(/datetime/i, "timestamp")
6095
mapping.register_type(/timestamp/i, timestamp)
6196

62-
assert_equal mapping.lookup("datetime"), timestamp
97+
assert_equal timestamp, mapping.lookup("datetime")
6398
end
6499

65100
def test_aliases_keep_metadata
66-
mapping = TypeMap.new
101+
mapping = klass.new
67102

68103
mapping.register_type(/decimal/i) { |sql_type| sql_type }
69104
mapping.alias_type(/number/i, "decimal")
@@ -72,10 +107,19 @@ def test_aliases_keep_metadata
72107
assert_equal mapping.lookup("number"), "decimal"
73108
end
74109

110+
def test_fuzzy_lookup
111+
string = +""
112+
mapping = klass.new
113+
114+
mapping.register_type(/varchar/i, string)
115+
116+
assert_equal mapping.lookup("varchar(20)"), string
117+
end
118+
75119
def test_register_proc
76120
string = +""
77121
binary = Binary.new
78-
mapping = TypeMap.new
122+
mapping = klass.new
79123

80124
mapping.register_type(/varchar/i) do |type|
81125
if type.include?("(")
@@ -89,29 +133,40 @@ def test_register_proc
89133
assert_equal mapping.lookup("varchar"), binary
90134
end
91135

136+
def test_parent_fallback
137+
boolean = Boolean.new
138+
139+
parent = klass.new
140+
parent.register_type(/boolean/i, boolean)
141+
142+
mapping = klass.new(parent)
143+
assert_equal boolean, mapping.lookup("boolean")
144+
end
145+
146+
private
147+
def klass
148+
TypeMap
149+
end
150+
end
151+
152+
class HashLookupTypeMapTest < ActiveRecord::TestCase
153+
include TypeMapSharedTests
154+
92155
def test_additional_lookup_args
93-
mapping = TypeMap.new
156+
mapping = HashLookupTypeMap.new
94157

95-
mapping.register_type(/varchar/i) do |type, limit|
158+
mapping.register_type("varchar") do |type, limit|
96159
if limit > 255
97160
"text"
98161
else
99162
"string"
100163
end
101164
end
102-
mapping.alias_type(/string/i, "varchar")
165+
mapping.alias_type("string", "varchar")
103166

104-
assert_equal mapping.lookup("varchar", 200), "string"
105-
assert_equal mapping.lookup("varchar", 400), "text"
106-
assert_equal mapping.lookup("string", 400), "text"
107-
end
108-
109-
def test_requires_value_or_block
110-
mapping = TypeMap.new
111-
112-
assert_raises(ArgumentError) do
113-
mapping.register_type(/only key/i)
114-
end
167+
assert_equal "string", mapping.lookup("varchar", 200)
168+
assert_equal "text", mapping.lookup("varchar", 400)
169+
assert_equal "text", mapping.lookup("string", 400)
115170
end
116171

117172
def test_lookup_non_strings
@@ -121,68 +176,31 @@ def test_lookup_non_strings
121176
mapping.register_type(2, "int")
122177
mapping.alias_type(3, 1)
123178

124-
assert_equal mapping.lookup(1), "string"
125-
assert_equal mapping.lookup(2), "int"
126-
assert_equal mapping.lookup(3), "string"
179+
assert_equal "string", mapping.lookup(1)
180+
assert_equal "int", mapping.lookup(2)
181+
assert_equal "string", mapping.lookup(3)
127182
assert_kind_of Type::Value, mapping.lookup(4)
128183
end
129184

130-
def test_fetch
131-
mapping = TypeMap.new
132-
mapping.register_type(1, "string")
133-
134-
assert_equal "string", mapping.fetch(1) { "int" }
135-
assert_equal "int", mapping.fetch(2) { "int" }
136-
end
137-
138-
def test_fetch_yields_args
139-
mapping = TypeMap.new
140-
141-
assert_equal "foo-1-2-3", mapping.fetch("foo", 1, 2, 3) { |*args| args.join("-") }
142-
assert_equal "bar-1-2-3", mapping.fetch("bar", 1, 2, 3) { |*args| args.join("-") }
143-
end
144-
145-
def test_fetch_memoizes
146-
mapping = TypeMap.new
147-
148-
looked_up = false
149-
mapping.register_type(1) do
150-
fail if looked_up
151-
looked_up = true
152-
"string"
153-
end
154-
155-
assert_equal "string", mapping.fetch(1)
156-
assert_equal "string", mapping.fetch(1)
157-
end
158-
159185
def test_fetch_memoizes_on_args
160-
mapping = TypeMap.new
186+
mapping = HashLookupTypeMap.new
161187
mapping.register_type("foo") { |*args| args.join("-") }
162188

163189
assert_equal "foo-1-2-3", mapping.fetch("foo", 1, 2, 3) { |*args| args.join("-") }
164190
assert_equal "foo-2-3-4", mapping.fetch("foo", 2, 3, 4) { |*args| args.join("-") }
165191
end
166192

167-
def test_register_clears_cache
168-
mapping = TypeMap.new
169-
170-
mapping.register_type(1, "string")
171-
mapping.lookup(1)
172-
mapping.register_type(1, "int")
193+
def test_fetch_yields_args
194+
mapping = klass.new
173195

174-
assert_equal "int", mapping.lookup(1)
196+
assert_equal "foo-1-2-3", mapping.fetch("foo", 1, 2, 3) { |*args| args.join("-") }
197+
assert_equal "bar-1-2-3", mapping.fetch("bar", 1, 2, 3) { |*args| args.join("-") }
175198
end
176199

177-
def test_parent_fallback
178-
boolean = Boolean.new
179-
180-
parent = TypeMap.new
181-
parent.register_type(/boolean/i, boolean)
182-
183-
mapping = TypeMap.new(parent)
184-
assert_equal mapping.lookup("boolean"), boolean
185-
end
200+
private
201+
def klass
202+
HashLookupTypeMap
203+
end
186204
end
187205
end
188206
end

0 commit comments

Comments
 (0)