-
Notifications
You must be signed in to change notification settings - Fork 0
Reimplement with django-readers #68
base: master
Are you sure you want to change the base?
Conversation
Thoughts on a plan for this going forward:
Below are approximate
becomes
becomes
becomes
becomes
becomes
becomes
|
Some of the SSM's 'specs' are a DSL for specifying endpoints declaratively. The implementation is there to allow this spec to be parsed to create the query so that it worries about implementation details such as N-queries problems so you don't have to. But the DSL you are putting into the user's hands is just as if not more important. I'd like SSM's DSL to stay as concise and expressive as it is -- or ideally, get MORE concise and expressive. I don't have any objection to its implementation being rewritten as you have done because there's nothing particularly pleasant about the existing one. But having any constraints in that implementation mean that the DSL loses conciseness or expressivity sounds like the tail wagging the dog and implementation driving the interface which I'm not really happy about. |
@pmg103 I think there's a balance to be struck. The SSM spec DSL is brilliant, but I think that if a DSL becomes too clever, it risks obscuring what's actually going on. Part of the point of That said, (and assuming the "long verbose python expressions" you're referring to are the
|
I think the thing to aim for is a nice composable fractal implementation that allows you to drop down a level simply and easily AND provide convenience functions for common patterns. That seems like the best balance. |
|
I've now moved a big chunk of the implementation into |
This rips out the guts of SSM and replaces it with an implementation based on
django-readers
.A few notes:
This is based on an as-yet-unreleased version ofdjango-readers
. PRs for all required features are open but not yet merged or released.Many of the tests that have changed are simply adding one extra query, because
django-readers
always usesprefetch_related
, whereas SSM sometimes cleverly usesselect_related
.An SSM spec is not the same thing as a django-readers spec. SSM includes a preprocessing step which supports extra syntax such as plugins
{"name": SomePlugin()}
,Aliased
,Filtered
etc. Each of these SSM-only concepts is converted to a pair, and the spec is then passed intospecs.process
as normal. It may be that this is not desirable in the long run, and we should deprecate plugins entirely. TBD.The feature of including a relationship name as a bare string in a spec, and having that be converted into a list of IDs of related objects, has been removed. This feature is undocumented in SSM as-is, would be very complicated to implement in
django-readers
and is (I believe) fairly surprising. Instead, the newpk_list
pair function indjango-readers
should be used instead. Replace"foo_set"
in your specs withpk_list("foo_set")
.This is a bit more subtle: this implementation doesn't actually really use
Serializer
at all. It uses a thing that quacks like aSerializer
(ie it has a.data
attribute) but in reality it's just calling thedjango-readers
projector function, which just plucks the field attributes directly off the model instances. This creates a slight a change in behaviour for some field types. A DRFUUIDField
, for example, converts aUUID
instance to a string itself in itsto_representation
method, so by the time it hits the renderer it is already just a JSON-renderable value (a string). The same is true for things likedate
anddatetime
.However, DRF's JSON encoder cleverly also has support for encoding lots of higher-level datatypes itself - so if you just return a
UUID
or adate
or adatetime
from your view then it will still be rendered correctly.The upshot of this is that if your tests are doing things like:
You'll need to change that to:
This is because
response.data
in a test is the raw data that comes back from the serializer, before it gets passed into the renderer.So this produces some test failures when installing this new version of SSM in a real project, but these are super easy to fix.
Slightly more complicated: if you're using some custom model fields that return something that isn't a JSON-serializable value from
to_python
, you'll need towrite a very simple custom plugin or pair to convert the value to a JSON-serializable type during projection. egtransform the value to a JSON-serializable type during projection, eg(qs.include_field("my_custom_field"), lambda instance: {"my_custom_field": str(instance.my_custom_field)})
. This could of course be wrapped up in a higher-order function that returns this pair.specs.field("my_custom_field", transform_value=str)
There is at least one actual bug still here (I think), despite the tests all passing. When installing this in a real project and running the tests, I get a fewTypeError: 'Filtered' object is not iterable
, which is a bit odd. Needs investigating.