Merge ~cjwatson/launchpad:immutable-pg-json into launchpad:master
- Git
- lp:~cjwatson/launchpad
- immutable-pg-json
- Merge into master
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | d6a6792e5c2647bddd4045a599dcae6c56dbfe63 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:immutable-pg-json |
Merge into: | launchpad:master |
Diff against target: |
605 lines (+216/-47) 17 files modified
lib/lp/buildmaster/browser/builder.py (+2/-2) lib/lp/buildmaster/interactor.py (+2/-10) lib/lp/buildmaster/interfaces/builder.py (+12/-3) lib/lp/buildmaster/interfaces/buildfarmjob.py (+2/-2) lib/lp/buildmaster/interfaces/buildqueue.py (+2/-2) lib/lp/buildmaster/model/builder.py (+5/-3) lib/lp/buildmaster/model/buildqueue.py (+5/-5) lib/lp/buildmaster/tests/mock_workers.py (+2/-3) lib/lp/buildmaster/tests/test_builder.py (+5/-5) lib/lp/code/browser/tests/test_gitrepository.py (+1/-1) lib/lp/code/interfaces/gitrepository.py (+11/-2) lib/lp/code/model/gitrepository.py (+5/-2) lib/lp/code/model/tests/test_cibuild.py (+2/-2) lib/lp/code/model/tests/test_gitnamespace.py (+1/-1) lib/lp/code/model/tests/test_gitrepository.py (+1/-1) lib/lp/services/database/stormexpr.py (+85/-0) lib/lp/services/database/tests/test_stormexpr.py (+73/-3) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andrey Fedoseev (community) | Approve | ||
William Grant | code | Approve | |
Review via email: mp+434104@code.launchpad.net |
Commit message
Use an immutable property for builder resources/
Description of the change
Adding builder resources and constraints using `storm.
Instead, introduce a new `ImmutablePgJSON` property, which is essentially a variant of `storm.
William Grant (wgrant) : | # |
Andrey Fedoseev (andrey-fedoseev) : | # |
Preview Diff
1 | diff --git a/lib/lp/buildmaster/browser/builder.py b/lib/lp/buildmaster/browser/builder.py |
2 | index 784eed4..1c24da5 100644 |
3 | --- a/lib/lp/buildmaster/browser/builder.py |
4 | +++ b/lib/lp/buildmaster/browser/builder.py |
5 | @@ -195,8 +195,8 @@ class BuilderSetView(CleanInfoMixin, LaunchpadView): |
6 | def getBuilderSortKey(builder): |
7 | return ( |
8 | not builder.virtualized, |
9 | - tuple(builder.restricted_resources or ()), |
10 | - tuple(builder.open_resources or ()), |
11 | + builder.restricted_resources or (), |
12 | + builder.open_resources or (), |
13 | tuple(p.name for p in builder.processors), |
14 | builder.name, |
15 | ) |
16 | diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py |
17 | index 00f90b1..174411c 100644 |
18 | --- a/lib/lp/buildmaster/interactor.py |
19 | +++ b/lib/lp/buildmaster/interactor.py |
20 | @@ -391,16 +391,8 @@ def extract_vitals_from_db(builder, build_queue=_BQ_UNSPECIFIED): |
21 | builder.virtualized, |
22 | builder.vm_host, |
23 | builder.vm_reset_protocol, |
24 | - ( |
25 | - None |
26 | - if builder.open_resources is None |
27 | - else tuple(builder.open_resources) |
28 | - ), |
29 | - ( |
30 | - None |
31 | - if builder.restricted_resources is None |
32 | - else tuple(builder.restricted_resources) |
33 | - ), |
34 | + builder.open_resources, |
35 | + builder.restricted_resources, |
36 | builder.builderok, |
37 | builder.manual, |
38 | build_queue, |
39 | diff --git a/lib/lp/buildmaster/interfaces/builder.py b/lib/lp/buildmaster/interfaces/builder.py |
40 | index 5a5d5f2..9225325 100644 |
41 | --- a/lib/lp/buildmaster/interfaces/builder.py |
42 | +++ b/lib/lp/buildmaster/interfaces/builder.py |
43 | @@ -34,7 +34,16 @@ from lazr.restful.declarations import ( |
44 | from lazr.restful.fields import Reference, ReferenceChoice |
45 | from lazr.restful.interface import copy_field |
46 | from zope.interface import Attribute, Interface |
47 | -from zope.schema import Bool, Choice, Datetime, Int, List, Text, TextLine |
48 | +from zope.schema import ( |
49 | + Bool, |
50 | + Choice, |
51 | + Datetime, |
52 | + Int, |
53 | + List, |
54 | + Text, |
55 | + TextLine, |
56 | + Tuple, |
57 | +) |
58 | |
59 | from lp import _ |
60 | from lp.app.validators.name import name_validator |
61 | @@ -217,7 +226,7 @@ class IBuilderView(IHasBuildRecords, IHasOwner): |
62 | ) |
63 | |
64 | open_resources = exported( |
65 | - List( |
66 | + Tuple( |
67 | title=_("Open resources"), |
68 | description=_( |
69 | "Resource tags offered by this builder, that can be required " |
70 | @@ -229,7 +238,7 @@ class IBuilderView(IHasBuildRecords, IHasOwner): |
71 | ) |
72 | |
73 | restricted_resources = exported( |
74 | - List( |
75 | + Tuple( |
76 | title=_("Restricted resources"), |
77 | description=_( |
78 | "Resource tags offered by this builder, indicating that the " |
79 | diff --git a/lib/lp/buildmaster/interfaces/buildfarmjob.py b/lib/lp/buildmaster/interfaces/buildfarmjob.py |
80 | index f05596f..0f516e5 100644 |
81 | --- a/lib/lp/buildmaster/interfaces/buildfarmjob.py |
82 | +++ b/lib/lp/buildmaster/interfaces/buildfarmjob.py |
83 | @@ -29,7 +29,7 @@ from lazr.restful.declarations import ( |
84 | ) |
85 | from lazr.restful.fields import Reference |
86 | from zope.interface import Attribute, Interface |
87 | -from zope.schema import Bool, Choice, Datetime, Int, List, TextLine, Timedelta |
88 | +from zope.schema import Bool, Choice, Datetime, Int, TextLine, Timedelta, Tuple |
89 | |
90 | from lp import _ |
91 | from lp.buildmaster.enums import BuildFarmJobType, BuildStatus |
92 | @@ -109,7 +109,7 @@ class IBuildFarmJobView(Interface): |
93 | ), |
94 | ) |
95 | |
96 | - builder_constraints = List( |
97 | + builder_constraints = Tuple( |
98 | title=_("Builder constraints"), |
99 | required=False, |
100 | readonly=True, |
101 | diff --git a/lib/lp/buildmaster/interfaces/buildqueue.py b/lib/lp/buildmaster/interfaces/buildqueue.py |
102 | index be1e4cd..824791f 100644 |
103 | --- a/lib/lp/buildmaster/interfaces/buildqueue.py |
104 | +++ b/lib/lp/buildmaster/interfaces/buildqueue.py |
105 | @@ -15,10 +15,10 @@ from zope.schema import ( |
106 | Choice, |
107 | Datetime, |
108 | Int, |
109 | - List, |
110 | Text, |
111 | TextLine, |
112 | Timedelta, |
113 | + Tuple, |
114 | ) |
115 | |
116 | from lp import _ |
117 | @@ -66,7 +66,7 @@ class IBuildQueue(Interface): |
118 | "The virtualization setting required by this build farm job." |
119 | ), |
120 | ) |
121 | - builder_constraints = List( |
122 | + builder_constraints = Tuple( |
123 | required=False, |
124 | value_type=TextLine(), |
125 | description=_( |
126 | diff --git a/lib/lp/buildmaster/model/builder.py b/lib/lp/buildmaster/model/builder.py |
127 | index d52551f..ea1a4ff 100644 |
128 | --- a/lib/lp/buildmaster/model/builder.py |
129 | +++ b/lib/lp/buildmaster/model/builder.py |
130 | @@ -8,7 +8,6 @@ __all__ = [ |
131 | ] |
132 | |
133 | import pytz |
134 | -from storm.databases.postgres import JSON |
135 | from storm.expr import Coalesce, Count, Sum |
136 | from storm.properties import Bool, DateTime, Int, Unicode |
137 | from storm.references import Reference |
138 | @@ -34,6 +33,7 @@ from lp.services.database.decoratedresultset import DecoratedResultSet |
139 | from lp.services.database.enumcol import DBEnum |
140 | from lp.services.database.interfaces import IStandbyStore, IStore |
141 | from lp.services.database.stormbase import StormBase |
142 | +from lp.services.database.stormexpr import ImmutablePgJSON |
143 | from lp.services.propertycache import cachedproperty, get_property_cache |
144 | |
145 | # XXX Michael Nelson 2010-01-13 bug=491330 |
146 | @@ -61,8 +61,10 @@ class Builder(StormBase): |
147 | virtualized = Bool(name="virtualized", default=True, allow_none=False) |
148 | manual = Bool(name="manual", default=False) |
149 | vm_host = Unicode(name="vm_host") |
150 | - open_resources = JSON(name="open_resources", allow_none=True) |
151 | - restricted_resources = JSON(name="restricted_resources", allow_none=True) |
152 | + open_resources = ImmutablePgJSON(name="open_resources", allow_none=True) |
153 | + restricted_resources = ImmutablePgJSON( |
154 | + name="restricted_resources", allow_none=True |
155 | + ) |
156 | active = Bool(name="active", allow_none=False, default=True) |
157 | failure_count = Int(name="failure_count", default=0, allow_none=False) |
158 | version = Unicode(name="version") |
159 | diff --git a/lib/lp/buildmaster/model/buildqueue.py b/lib/lp/buildmaster/model/buildqueue.py |
160 | index c69cc65..1bbcd03 100644 |
161 | --- a/lib/lp/buildmaster/model/buildqueue.py |
162 | +++ b/lib/lp/buildmaster/model/buildqueue.py |
163 | @@ -13,7 +13,6 @@ from itertools import groupby |
164 | from operator import attrgetter |
165 | |
166 | import pytz |
167 | -from storm.databases.postgres import JSON |
168 | from storm.expr import SQL, Cast, Coalesce, Desc, Exists, Or |
169 | from storm.properties import Bool, DateTime, Int, TimeDelta, Unicode |
170 | from storm.references import Reference |
171 | @@ -35,7 +34,7 @@ from lp.services.database.constants import DEFAULT, UTC_NOW |
172 | from lp.services.database.enumcol import DBEnum |
173 | from lp.services.database.interfaces import IStore |
174 | from lp.services.database.stormbase import StormBase |
175 | -from lp.services.database.stormexpr import JSONContains |
176 | +from lp.services.database.stormexpr import ImmutablePgJSON, JSONContains |
177 | from lp.services.features import getFeatureFlag |
178 | from lp.services.propertycache import cachedproperty, get_property_cache |
179 | |
180 | @@ -101,7 +100,9 @@ class BuildQueue(StormBase): |
181 | processor_id = Int(name="processor") |
182 | processor = Reference(processor_id, "Processor.id") |
183 | virtualized = Bool(name="virtualized") |
184 | - builder_constraints = JSON(name="builder_constraints", allow_none=True) |
185 | + builder_constraints = ImmutablePgJSON( |
186 | + name="builder_constraints", allow_none=True |
187 | + ) |
188 | |
189 | @property |
190 | def specific_source(self): |
191 | @@ -355,8 +356,7 @@ class BuildQueueSet: |
192 | JSONContains( |
193 | Cast( |
194 | json.dumps( |
195 | - tuple(open_resources or ()) |
196 | - + tuple(restricted_resources or ()) |
197 | + (open_resources or ()) + (restricted_resources or ()) |
198 | ), |
199 | "jsonb", |
200 | ), |
201 | diff --git a/lib/lp/buildmaster/tests/mock_workers.py b/lib/lp/buildmaster/tests/mock_workers.py |
202 | index 01c9732..55ac82d 100644 |
203 | --- a/lib/lp/buildmaster/tests/mock_workers.py |
204 | +++ b/lib/lp/buildmaster/tests/mock_workers.py |
205 | @@ -22,7 +22,6 @@ import shlex |
206 | import sys |
207 | import xmlrpc.client |
208 | from collections import OrderedDict |
209 | -from copy import copy |
210 | from textwrap import dedent |
211 | |
212 | try: |
213 | @@ -81,8 +80,8 @@ class MockBuilder: |
214 | self.virtualized = virtualized |
215 | self.vm_host = vm_host |
216 | self.vm_reset_protocol = vm_reset_protocol |
217 | - self.open_resources = copy(open_resources) |
218 | - self.restricted_resources = copy(restricted_resources) |
219 | + self.open_resources = open_resources |
220 | + self.restricted_resources = restricted_resources |
221 | self.failnotes = None |
222 | self.version = version |
223 | self.clean_status = clean_status |
224 | diff --git a/lib/lp/buildmaster/tests/test_builder.py b/lib/lp/buildmaster/tests/test_builder.py |
225 | index e5af5cb..39da0c8 100644 |
226 | --- a/lib/lp/buildmaster/tests/test_builder.py |
227 | +++ b/lib/lp/buildmaster/tests/test_builder.py |
228 | @@ -240,7 +240,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase): |
229 | processor=das.processor, |
230 | virtualized=True, |
231 | limit=5, |
232 | - open_resources=["large"], |
233 | + open_resources=("large",), |
234 | ), |
235 | ) |
236 | self.assertContentEqual( |
237 | @@ -249,7 +249,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase): |
238 | processor=das.processor, |
239 | virtualized=True, |
240 | limit=5, |
241 | - open_resources=["large", "gpu"], |
242 | + open_resources=("large", "gpu"), |
243 | ), |
244 | ) |
245 | self.assertEqual( |
246 | @@ -258,7 +258,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase): |
247 | processor=das.processor, |
248 | virtualized=True, |
249 | limit=5, |
250 | - restricted_resources=["gpu"], |
251 | + restricted_resources=("gpu",), |
252 | ), |
253 | ) |
254 | self.assertEqual( |
255 | @@ -267,8 +267,8 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase): |
256 | processor=das.processor, |
257 | virtualized=True, |
258 | limit=5, |
259 | - open_resources=["large"], |
260 | - restricted_resources=["gpu"], |
261 | + open_resources=("large",), |
262 | + restricted_resources=("gpu",), |
263 | ), |
264 | ) |
265 | |
266 | diff --git a/lib/lp/code/browser/tests/test_gitrepository.py b/lib/lp/code/browser/tests/test_gitrepository.py |
267 | index 1c5120c..51bf64b 100644 |
268 | --- a/lib/lp/code/browser/tests/test_gitrepository.py |
269 | +++ b/lib/lp/code/browser/tests/test_gitrepository.py |
270 | @@ -1119,7 +1119,7 @@ class TestGitRepositoryEditBuilderConstraintsView(BrowserTestCase): |
271 | browser.getControl("Change Git Repository").click() |
272 | |
273 | login_person(owner) |
274 | - self.assertEqual(["gpu", "large"], repository.builder_constraints) |
275 | + self.assertEqual(("gpu", "large"), repository.builder_constraints) |
276 | |
277 | |
278 | class TestGitRepositoryEditView(TestCaseWithFactory): |
279 | diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py |
280 | index a955c0c..2bbbb6b 100644 |
281 | --- a/lib/lp/code/interfaces/gitrepository.py |
282 | +++ b/lib/lp/code/interfaces/gitrepository.py |
283 | @@ -43,7 +43,16 @@ from lazr.restful.fields import CollectionField, Reference |
284 | from lazr.restful.interface import copy_field |
285 | from zope.component import getUtility |
286 | from zope.interface import Attribute, Interface |
287 | -from zope.schema import Bool, Choice, Datetime, Int, List, Text, TextLine |
288 | +from zope.schema import ( |
289 | + Bool, |
290 | + Choice, |
291 | + Datetime, |
292 | + Int, |
293 | + List, |
294 | + Text, |
295 | + TextLine, |
296 | + Tuple, |
297 | +) |
298 | |
299 | from lp import _ |
300 | from lp.app.enums import InformationType |
301 | @@ -1247,7 +1256,7 @@ class IGitRepositoryAdminAttributes(Interface): |
302 | """ |
303 | |
304 | builder_constraints = exported( |
305 | - List( |
306 | + Tuple( |
307 | title=_("Builder constraints"), |
308 | required=False, |
309 | readonly=False, |
310 | diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py |
311 | index bd09864..b1a8c28 100644 |
312 | --- a/lib/lp/code/model/gitrepository.py |
313 | +++ b/lib/lp/code/model/gitrepository.py |
314 | @@ -24,7 +24,7 @@ from breezy import urlutils |
315 | from lazr.enum import DBItem |
316 | from lazr.lifecycle.event import ObjectModifiedEvent |
317 | from lazr.lifecycle.snapshot import Snapshot |
318 | -from storm.databases.postgres import JSON, Returning |
319 | +from storm.databases.postgres import Returning |
320 | from storm.expr import SQL, And, Coalesce, Desc, Insert, Join, Not, Or, Select |
321 | from storm.info import ClassAlias, get_cls_info |
322 | from storm.locals import Bool, DateTime, Int, Reference, Unicode |
323 | @@ -143,6 +143,7 @@ from lp.services.database.stormexpr import ( |
324 | ArrayAgg, |
325 | ArrayIntersects, |
326 | BulkUpdate, |
327 | + ImmutablePgJSON, |
328 | Values, |
329 | ) |
330 | from lp.services.features import getFeatureFlag |
331 | @@ -323,7 +324,9 @@ class GitRepository( |
332 | name="date_last_scanned", tzinfo=pytz.UTC, allow_none=True |
333 | ) |
334 | |
335 | - builder_constraints = JSON(name="builder_constraints", allow_none=True) |
336 | + builder_constraints = ImmutablePgJSON( |
337 | + name="builder_constraints", allow_none=True |
338 | + ) |
339 | |
340 | def __init__( |
341 | self, |
342 | diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py |
343 | index 84ca9e1..114b606 100644 |
344 | --- a/lib/lp/code/model/tests/test_cibuild.py |
345 | +++ b/lib/lp/code/model/tests/test_cibuild.py |
346 | @@ -168,7 +168,7 @@ class TestCIBuild(TestCaseWithFactory): |
347 | ) |
348 | bq = build.queueBuild() |
349 | self.assertProvides(bq, IBuildQueue) |
350 | - self.assertEqual(["gpu"], bq.builder_constraints) |
351 | + self.assertEqual(("gpu",), bq.builder_constraints) |
352 | |
353 | def test_is_private(self): |
354 | # A CIBuild is private iff its repository is. |
355 | @@ -777,7 +777,7 @@ class TestCIBuildSet(TestCaseWithFactory): |
356 | build = getUtility(ICIBuildSet).requestBuild( |
357 | repository, commit_sha1, das, [[("test", 0)]] |
358 | ) |
359 | - self.assertEqual(["gpu"], build.builder_constraints) |
360 | + self.assertEqual(("gpu",), build.builder_constraints) |
361 | |
362 | def test_requestBuildsForRefs_triggers_builds(self): |
363 | ubuntu = getUtility(ILaunchpadCelebrities).ubuntu |
364 | diff --git a/lib/lp/code/model/tests/test_gitnamespace.py b/lib/lp/code/model/tests/test_gitnamespace.py |
365 | index c9c92b0..97bd2a8 100644 |
366 | --- a/lib/lp/code/model/tests/test_gitnamespace.py |
367 | +++ b/lib/lp/code/model/tests/test_gitnamespace.py |
368 | @@ -158,7 +158,7 @@ class NamespaceMixin: |
369 | repository_name, |
370 | builder_constraints=["gpu"], |
371 | ) |
372 | - self.assertEqual(["gpu"], repository.builder_constraints) |
373 | + self.assertEqual(("gpu",), repository.builder_constraints) |
374 | |
375 | def test_getRepositories_no_repositories(self): |
376 | # getRepositories on an IGitNamespace returns a result set of |
377 | diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py |
378 | index 2239439..8fd9c9f 100644 |
379 | --- a/lib/lp/code/model/tests/test_gitrepository.py |
380 | +++ b/lib/lp/code/model/tests/test_gitrepository.py |
381 | @@ -6621,7 +6621,7 @@ class TestGitRepositoryWebservice(TestCaseWithFactory): |
382 | ) |
383 | self.assertEqual(209, response.status) |
384 | with person_logged_in(ANONYMOUS): |
385 | - self.assertEqual(["gpu"], repository_db.builder_constraints) |
386 | + self.assertEqual(("gpu",), repository_db.builder_constraints) |
387 | |
388 | def test_builder_constraints_owner(self): |
389 | # The owner of a repository cannot change its builder constraints |
390 | diff --git a/lib/lp/services/database/stormexpr.py b/lib/lp/services/database/stormexpr.py |
391 | index d9b25d6..f9e808f 100644 |
392 | --- a/lib/lp/services/database/stormexpr.py |
393 | +++ b/lib/lp/services/database/stormexpr.py |
394 | @@ -14,6 +14,7 @@ __all__ = [ |
395 | "fti_search", |
396 | "Greatest", |
397 | "get_where_for_reference", |
398 | + "ImmutablePgJSON", |
399 | "IsDistinctFrom", |
400 | "IsFalse", |
401 | "IsTrue", |
402 | @@ -30,6 +31,9 @@ __all__ = [ |
403 | "WithMaterialized", |
404 | ] |
405 | |
406 | +import json |
407 | +from types import MappingProxyType |
408 | + |
409 | from storm import Undef |
410 | from storm.exceptions import ClassInfoError |
411 | from storm.expr import ( |
412 | @@ -49,6 +53,8 @@ from storm.expr import ( |
413 | compile, |
414 | ) |
415 | from storm.info import get_cls_info, get_obj_info |
416 | +from storm.properties import SimpleProperty |
417 | +from storm.variables import Variable |
418 | |
419 | |
420 | class BulkUpdate(Expr): |
421 | @@ -324,6 +330,85 @@ def compile_with_materialized(compile, with_expr, state): |
422 | return "".join(tokens) |
423 | |
424 | |
425 | +class ImmutablePgJSONVariable(Variable): |
426 | + """An immutable version of `storm.databases.postgres.JSONVariable`. |
427 | + |
428 | + Storm can't easily detect when variables that contain references to |
429 | + mutable content have been changed, and has to work around this by |
430 | + checking the variable's state when the store is flushing current |
431 | + objects. Unfortunately, this means that if there's a large number of |
432 | + live objects containing mutable-value properties, then flushes become |
433 | + very slow as Storm has to dump many objects to check whether they've |
434 | + changed. |
435 | + |
436 | + This class works around this performance problem by disabling the |
437 | + mutable-value tracking, at the expense of users no longer being able to |
438 | + mutate the value in place. Any mutations must be implemented by |
439 | + assigning to the property, rather than by mutating its value. |
440 | + """ |
441 | + |
442 | + __slots__ = () |
443 | + |
444 | + def get_state(self): |
445 | + return self._lazy_value, self._dumps(self._value) |
446 | + |
447 | + def set_state(self, state): |
448 | + self._lazy_value = state[0] |
449 | + self._value = self._loads(state[1]) |
450 | + |
451 | + def _make_immutable(self, value): |
452 | + """Return an immutable version of `value`. |
453 | + |
454 | + This only supports those types that `json.loads` can return as part |
455 | + of a decoded object. |
456 | + """ |
457 | + if isinstance(value, list): |
458 | + return tuple(self._make_immutable(item) for item in value) |
459 | + elif isinstance(value, dict): |
460 | + return MappingProxyType( |
461 | + {k: self._make_immutable(v) for k, v in value.items()} |
462 | + ) |
463 | + else: |
464 | + return value |
465 | + |
466 | + def parse_set(self, value, from_db): |
467 | + if from_db: |
468 | + value = self._loads(value) |
469 | + return self._make_immutable(value) |
470 | + |
471 | + def parse_get(self, value, to_db): |
472 | + if to_db: |
473 | + return self._dumps(value) |
474 | + else: |
475 | + return value |
476 | + |
477 | + def _loads(self, value): |
478 | + # psycopg2 >= 2.5 automatically decodes JSON columns for us. |
479 | + return value |
480 | + |
481 | + def _dumps(self, value): |
482 | + # See https://github.com/python/cpython/issues/79039. |
483 | + def default(value): |
484 | + if isinstance(value, MappingProxyType): |
485 | + return value.copy() |
486 | + else: |
487 | + raise TypeError( |
488 | + "Object of type %s is not JSON serializable" |
489 | + % value.__class__.__name__ |
490 | + ) |
491 | + |
492 | + return json.dumps(value, ensure_ascii=False, default=default) |
493 | + |
494 | + |
495 | +class ImmutablePgJSON(SimpleProperty): |
496 | + """An immutable version of `storm.databases.postgres.JSON`. |
497 | + |
498 | + See `ImmutablePgJSONVariable`. |
499 | + """ |
500 | + |
501 | + variable_class = ImmutablePgJSONVariable |
502 | + |
503 | + |
504 | def get_where_for_reference(reference, other): |
505 | """Generate a column comparison expression for a reference property. |
506 | |
507 | diff --git a/lib/lp/services/database/tests/test_stormexpr.py b/lib/lp/services/database/tests/test_stormexpr.py |
508 | index 61eb4a3..c8a803d 100644 |
509 | --- a/lib/lp/services/database/tests/test_stormexpr.py |
510 | +++ b/lib/lp/services/database/tests/test_stormexpr.py |
511 | @@ -3,9 +3,11 @@ |
512 | |
513 | """Test custom Storm expressions.""" |
514 | |
515 | -from types import SimpleNamespace |
516 | +from types import MappingProxyType, SimpleNamespace |
517 | |
518 | -from storm.expr import Select |
519 | +from storm.exceptions import NoneError |
520 | +from storm.expr import Column, Select |
521 | +from storm.info import get_obj_info |
522 | from zope.component import getUtility |
523 | |
524 | from lp.services.database.interfaces import ( |
525 | @@ -14,7 +16,11 @@ from lp.services.database.interfaces import ( |
526 | IStoreSelector, |
527 | ) |
528 | from lp.services.database.sqlbase import convert_storm_clause_to_string |
529 | -from lp.services.database.stormexpr import WithMaterialized |
530 | +from lp.services.database.stormexpr import ( |
531 | + ImmutablePgJSON, |
532 | + ImmutablePgJSONVariable, |
533 | + WithMaterialized, |
534 | +) |
535 | from lp.testing import TestCase |
536 | from lp.testing.layers import BaseLayer, ZopelessDatabaseLayer |
537 | |
538 | @@ -64,3 +70,67 @@ class TestWithMaterializedRealDatabase(TestCase): |
539 | "test AS MATERIALIZED (SELECT 1)", |
540 | ], |
541 | ) |
542 | + |
543 | + |
544 | +class TestImmutablePgJSON(TestCase): |
545 | + |
546 | + layer = BaseLayer |
547 | + |
548 | + def setUpProperty(self, *args, **kwargs): |
549 | + class Class: |
550 | + __storm_table__ = "mytable" |
551 | + prop1 = ImmutablePgJSON("column1", *args, primary=True, **kwargs) |
552 | + |
553 | + class Subclass(Class): |
554 | + pass |
555 | + |
556 | + self.Class = Class |
557 | + self.Subclass = Subclass |
558 | + self.obj = Subclass() |
559 | + self.obj_info = get_obj_info(self.obj) |
560 | + self.column1 = self.Subclass.prop1 |
561 | + self.variable1 = self.obj_info.variables[self.column1] |
562 | + |
563 | + def test_basic(self): |
564 | + self.setUpProperty(default_factory=dict, allow_none=False) |
565 | + self.assertIsInstance(self.column1, Column) |
566 | + self.assertEqual("column1", self.column1.name) |
567 | + self.assertEqual(self.Subclass, self.column1.table) |
568 | + self.assertIsInstance(self.variable1, ImmutablePgJSONVariable) |
569 | + |
570 | + def test_immutable_default(self): |
571 | + self.setUpProperty(default_factory=dict, allow_none=False) |
572 | + self.assertIsInstance(self.obj.prop1, MappingProxyType) |
573 | + self.assertEqual({}, self.obj.prop1) |
574 | + self.assertEqual("{}", self.variable1.get(to_db=True)) |
575 | + self.assertRaises(NoneError, setattr, self.obj, "prop1", None) |
576 | + self.assertRaises( |
577 | + AttributeError, getattr, self.obj.prop1, "__setitem__" |
578 | + ) |
579 | + |
580 | + def test_immutable_dict(self): |
581 | + self.setUpProperty() |
582 | + self.variable1.set({"a": {"b": []}}, from_db=True) |
583 | + self.assertIsInstance(self.obj.prop1, MappingProxyType) |
584 | + self.assertIsInstance(self.obj.prop1["a"], MappingProxyType) |
585 | + self.assertIsInstance(self.obj.prop1["a"]["b"], tuple) |
586 | + self.assertEqual({"a": {"b": ()}}, self.obj.prop1) |
587 | + self.assertEqual('{"a": {"b": []}}', self.variable1.get(to_db=True)) |
588 | + self.assertRaises( |
589 | + AttributeError, getattr, self.obj.prop1, "__setitem__" |
590 | + ) |
591 | + self.obj.prop1 = {"a": 1} |
592 | + self.assertIsInstance(self.obj.prop1, MappingProxyType) |
593 | + self.assertEqual({"a": 1}, self.obj.prop1) |
594 | + self.assertEqual('{"a": 1}', self.variable1.get(to_db=True)) |
595 | + |
596 | + def test_immutable_list(self): |
597 | + self.setUpProperty() |
598 | + self.variable1.set([], from_db=True) |
599 | + self.assertIsInstance(self.obj.prop1, tuple) |
600 | + self.assertEqual((), self.obj.prop1) |
601 | + self.assertEqual("[]", self.variable1.get(to_db=True)) |
602 | + self.obj.prop1 = ["a"] |
603 | + self.assertIsInstance(self.obj.prop1, tuple) |
604 | + self.assertEqual(("a",), self.obj.prop1) |
605 | + self.assertEqual('["a"]', self.variable1.get(to_db=True)) |