feat: Add custom field (metadata) support - #23
Conversation
0055c7c to
7cca891
Compare
961dedb to
a9d977b
Compare
7cca891 to
b617e75
Compare
96dd804 to
e673aa8
Compare
e673aa8 to
0517426
Compare
0517426 to
3cd0a6d
Compare
801dcee to
fd5b450
Compare
3cd0a6d to
b69421f
Compare
Adds support for reading and writing Flagsmith custom fields on features,
segments and environments, which terraform-provider-flagsmith#215 needs. Until
now the provider could not set a custom field at all, so an organisation with a
mandatory custom field could not create features through it.
The API takes a MetadataModelField ID, which is not the same as the custom
field's own ID: a single field bound to features, segments and environments has
three of them. Resolving a name to the right one is fiddly enough that it does
not belong in every caller, so this adds a MetadataFieldResolver that maps names
to model field IDs for one (project, entity) pair and back again.
Two API details drive the design:
- `GET /projects/{id}/metadata/fields/?entity=feature` filters which *fields*
are returned but does NOT filter each field's nested `model_fields`, so the
caller has to match on content type regardless. The entity param is therefore
not used, which also lets one response serve all three entity types.
- Django content type IDs are specific to an installation, so they are resolved
at runtime from the supported-content-types endpoint and never hardcoded.
`GET /features/get-by-uuid/` is served by CreateFeatureSerializer, which omits
metadata, so GetFeature hydrates it from the project scoped retrieve endpoint.
That costs one extra request per feature read. Segments and environments need no
workaround: their get-by-uuid responses already include metadata. See the TODO
for the upstream fix that would remove this.
Feature.UnmarshalJSON decodes into an explicit allowlist, so adding Metadata to
the struct alone would have silently dropped it on every read. The allowlist now
carries a warning, and a reflection based test fails if a future field is
missed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b69421f to
394616d
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe client adds metadata models, field validation, typed errors, paginated field retrieval, content-type lookup, and entity-specific field resolution. It can build metadata from named values and resolve model-field identifiers back to names. Feature, segment, and environment models now expose metadata. Feature retrieval hydrates metadata through a project-scoped endpoint. Tests cover API requests, validation, serialisation, resolution, and resource round trips. Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This change adds metadata hydration to feature reads, but a 403 or 404 from the metadata endpoint currently causes the entire feature request to fail instead of returning the feature without metadata. That availability regression should be addressed or explicitly accepted before merging; the other risks are bounded follow-up items. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3aadaf4-8e18-44e1-a38b-f64bffebcec1
📒 Files selected for processing (5)
client.goerrors.gometadata.gometadata_test.gomodels.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Count value lengths in runes rather than bytes. The API validates with Python's len on a str, which counts characters, so a value of 2000 multi byte characters is valid there and was being rejected here. Report a not-found rather than a metadata error when a feature is deleted between the get-by-uuid read and the metadata read, so the caller can treat it the same way as any other missing feature. Drop the trailing empty list from MetadataFieldNotFoundError when no custom field is enabled for the entity at all, which is the most likely case for a project that has none. Add request body assertions for the segment and environment write paths. Only the feature create path had one, and sending a model field ID bound to the wrong entity type is the failure mode most worth guarding against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Responses to the CodeRabbit review. Threaded replies aren't possible while there's a pending review on the PR, so they're collected here.
Not swallowing 403 — both endpoints are gated by the same
|
Drops the TODO on the feature metadata hydration, the doc comments on the metadata error types, and the warning above the Feature.UnmarshalJSON allowlist. The allowlist is guarded by a test whose failure message names the fix, so the comment was duplicating it. Also drops a handful of comments in metadata.go that only restated the name of the thing below them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mapping a 404 from the metadata read to FeatureNotFoundError only mattered if a feature was deleted between two back to back requests, and GetFeature already has the same unguarded race one line earlier with GetProjectByID. It also meant passing a feature UUID into getFeatureMetadata solely to build the error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Building a resolver costs three requests, and callers need one per entity they write, so a Terraform run over many features in one project repeated that for every resource. Caches the resolver on the client, keyed on project and entity. Scoped to the resolver itself rather than the three responses behind it: a second entity type in the same project is built separately, which keeps this to one map and one mutex while still collapsing the common case of many resources of the same kind. Failures are not cached, so a transient error cannot poison the client. The cache lives for the lifetime of the client, which for the Terraform provider is a single plan or apply. A custom field created after the first resolver for a project will not be seen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Adds read and write support for Flagsmith custom fields on features, segments and environments.
Unblocks terraform-provider-flagsmith#215: the provider can't set a custom field at all today, so an organisation with a mandatory one can't create features through Terraform.
Resolving field names
The API takes a
MetadataModelFieldID, not the custom field's own ID — a field bound to features, segments and environments has three.MetadataFieldResolverhandles that for one(project, entity)pair, withBuildMetadata/ResolveMetadataNamesfor the map-of-names case. Building one costs three requests — the project, its organisation's content types, and the project's field definitions — and nothing is cached:Two API details shape this, both commented in the code:
?entity=featurefilters which fields come back, but not their nestedmodel_fields, so the caller has to match on content type anyway. The param is unused, so one response serves all three entity types.supported-content-typesrather than hardcoded.Feature reads cost one more request
GET /features/get-by-uuid/usesCreateFeatureSerializer, which omitsmetadata, soGetFeaturehydrates it from the project-scoped retrieve endpoint. That's one extra request per feature read. There's aTODOfor the upstream fix that would remove the hydration entirely. Segments and environments need no workaround.Feature.UnmarshalJSONIt decodes via an explicit allowlist, so adding
Metadatato the struct alone would have silently dropped it on every response. The allowlist now carries a warning, andTestFeatureUnmarshalJSONDoesNotDropFieldsreflects over the struct so the next field can't be missed the same way.🤖 Generated with Claude Code