Ensure field extensions are only applied once #3832
Conversation
Reviewer's Guide by SourceryThe pull request introduces a caching mechanism to ensure that field extensions are applied only once, even if a field is used in multiple schemas. This is achieved by adding a new utility function Sequence diagram for applying field extensions with cachingsequenceDiagram
participant SchemaConverter
participant Field
participant FieldExtension
SchemaConverter->>Field: Get field extensions
SchemaConverter->>apply_field_extensions: Call
apply_field_extensions->>Field: Check cache
alt Cache miss
apply_field_extensions->>FieldExtension: Apply each extension
FieldExtension-->>apply_field_extensions: Extension applied
apply_field_extensions-->>Field: Cache result
else Cache hit
apply_field_extensions-->>Field: Return cached result
end
Class diagram for the updated field extension applicationclassDiagram
class StrawberryField {
+List~FieldExtension~ extensions
}
class FieldExtension {
+apply(field: StrawberryField)
}
class SchemaConverter {
+wrap_field_extensions()
}
class Utility {
+apply_field_extensions(field: StrawberryField)
}
StrawberryField --> FieldExtension : uses
SchemaConverter --> Utility : calls
Utility --> FieldExtension : applies
note for Utility "apply_field_extensions is cached to ensure single application"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey! Thanks for the PR!
I wonder if we can maybe scope the cache to the schema? Or maybe when applying the extensions we make a copy of the field?
Just think about a future in which we might have default field extensions (based on the schema) and fields on different schemas will have different fields, what do you think?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3832 +/- ##
==========================================
- Coverage 95.14% 95.13% -0.02%
==========================================
Files 500 500
Lines 32463 32494 +31
Branches 1684 1685 +1
==========================================
+ Hits 30886 30912 +26
- Misses 1311 1315 +4
- Partials 266 267 +1 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3832 will not alter performanceComparing Summary
|
Was thinking that exactly! Scope the cache to the schema somehow (either by having it as a Copying the field is an option, but might generate other issues? |
Hi @patrick91 & @bellini666 , thanks so much for your replies! I actually considered both of these for this PR, and decided against it for my initial implementation - hopefully I can explain why below. 1. Scoping the cache to the schemaMaybe I'm missing something, but I don't think this will actually work in isolation. The issue we're trying to solve here isn't that a single schema will apply the extensions multiple times - its that currently the same field instances are shared across multiple schemas, so each schema will apply the field extensions once. So, if we cache the extensions per-schema, we will still end up applying field extensions to the same field multiple times (once per-schema). However, I do agree with the reasoning here - if in the future we have default field extensions that are schema-specific, we may need to cache them per-schema. 2. Copying the field before applying the extensionsThis does seem to be a valid approach, and I think it would work (and would even work in conjunction with a schema-scoped cache). However, I was worried about whether this might be a bit of a hack. Do we know if there are any flow-on effects of doing this? For example, is there code relying on there being only one instance of a field per schema, or code that relies on the field being the same instance after schema generation? Maybe there is even downstream code relying on this? If we aren't concerned about that - and copying fields before applying extensions is valid - then maybe this is the way to go? Does my reasoning above make sense and/or give you any ideas? Or am I missing something in my understanding above? 😅 |
|
Sorry for the immediate follow-up, but I've got some extra context that might influence our decision / implementation. I looked into copying fields in my project, and I've realised that I have custom class MyCustomStrawberryField(StrawberryField):
def __init__(self, *args, my_custom_arg, **kwargs):
super().__init__(*args, **kwargs)
self.something = self.do_some_processing_with(my_custom_arg)This example subclass:
I have also found that the base Regardless, this is probably a design issue with my subclass - but its made me wonder whether similar downstream issues might be encountered if we choose to copy the fields here? |
Description
Adds the
apply_field_extensionsutility function which uses@cacheto ensure field extensions are only ever applied once, regardless of the number of schemas that use the field.Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Prevent multiple applications of field extensions across different schema generations
New Features:
apply_field_extensionsutility function with@cachedecorator to ensure field extensions are applied only onceBug Fixes:
Tests: