-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add DynamoDB document schemas #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: ci/uv-renovate
Are you sure you want to change the base?
Changes from all commits
8b18b1a
815f0d3
7c45ffb
9d21d93
a29f199
4d8c83a
ac9e8df
0179e00
c743785
d014c4a
37a6acd
581de51
0b438e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,4 +4,5 @@ | |
| *.pyc | ||
| .coverage | ||
| common.sqlite3 | ||
| dist/ | ||
| dist/ | ||
| coverage.xml | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,331 @@ | ||
| """ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this module is pretty generic - doesn't tell us what these models are for. Something like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great suggestion! Renamed to |
||
| The types in this module describe the Edge API's data model. | ||
| They are used to type DynamoDB documents representing Flagsmith entities. | ||
| """ | ||
|
|
||
| from typing import Literal | ||
|
|
||
| from typing_extensions import NotRequired, TypedDict | ||
|
|
||
| from flagsmith_schemas.types import ( | ||
| ConditionOperator, | ||
| ContextValue, | ||
| DateTimeStr, | ||
| DynamoFloat, | ||
| DynamoInt, | ||
| FeatureType, | ||
| FeatureValue, | ||
| RuleType, | ||
| UUIDStr, | ||
| ) | ||
|
|
||
|
|
||
| class Feature(TypedDict): | ||
| """Represents a Flagsmith feature, defined at project level.""" | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the feature in Core.""" | ||
| name: str | ||
| """Name of the feature. Must be unique within a project.""" | ||
| type: FeatureType | ||
|
|
||
|
|
||
| class MultivariateFeatureOption(TypedDict): | ||
| """Represents a single multivariate feature option in the Flagsmith UI.""" | ||
|
|
||
| id: NotRequired[DynamoInt | None] | ||
| """Unique identifier for the multivariate feature option in Core. **DEPRECATED**: MultivariateFeatureValue.id should be used instead.""" | ||
| value: FeatureValue | ||
| """The feature state value that should be served when this option's parent multivariate feature state is selected by the engine.""" | ||
|
|
||
|
|
||
| class MultivariateFeatureStateValue(TypedDict): | ||
| """Represents a multivariate feature state value. | ||
|
|
||
| Identity overrides are meant to hold only one of these, solely to inform the UI which option is selected for the given identity. | ||
| """ | ||
|
|
||
| id: NotRequired[DynamoInt | None] | ||
| """Unique identifier for the multivariate feature state value in Core. If feature state created via `edge-identities` APIs in Core, this can be missing or `None`.""" | ||
| mv_fs_value_uuid: NotRequired[UUIDStr] | ||
| """The UUID for this multivariate feature state value. Should be used if `id` is `None`.""" | ||
| percentage_allocation: DynamoFloat | ||
| """The percentage allocation for this multivariate feature state value. Should be between or equal to 0 and 100.""" | ||
| multivariate_feature_option: MultivariateFeatureOption | ||
| """The multivariate feature option that this value corresponds to.""" | ||
|
|
||
|
|
||
| class FeatureSegment(TypedDict): | ||
| """Represents data specific to a segment feature override.""" | ||
|
|
||
| priority: NotRequired[DynamoInt | None] | ||
| """The priority of this segment feature override. Lower numbers indicate stronger priority. If `None` or not set, the weakest priority is assumed.""" | ||
|
|
||
|
|
||
| class FeatureState(TypedDict): | ||
| """Used to define the state of a feature for an environment, segment overrides, and identity overrides.""" | ||
|
|
||
| feature: Feature | ||
| """The feature that this feature state is for.""" | ||
| enabled: bool | ||
| """Whether the feature is enabled or disabled.""" | ||
| feature_state_value: FeatureValue | ||
| """The value for this feature state.""" | ||
| django_id: NotRequired[DynamoInt | None] | ||
| """Unique identifier for the feature state in Core. If feature state created via Core's `edge-identities` APIs in Core, this can be missing or `None`.""" | ||
| featurestate_uuid: NotRequired[UUIDStr] | ||
| """The UUID for this feature state. Should be used if `django_id` is `None`. If not set, should be generated.""" | ||
| feature_segment: NotRequired[FeatureSegment | None] | ||
| """Segment override data, if this feature state is for a segment override.""" | ||
| multivariate_feature_state_values: NotRequired[list[MultivariateFeatureStateValue]] | ||
| """List of multivariate feature state values, if this feature state is for a multivariate feature. | ||
|
|
||
| Total `percentage_allocation` sum must be less or equal to 100. | ||
| """ | ||
|
|
||
|
|
||
| class Trait(TypedDict): | ||
| """Represents a key-value pair associated with an identity.""" | ||
|
|
||
| trait_key: str | ||
| """Key of the trait.""" | ||
| trait_value: ContextValue | ||
| """Value of the trait.""" | ||
|
|
||
|
|
||
| class SegmentCondition(TypedDict): | ||
| """Represents a condition within a segment rule used by Flagsmith engine.""" | ||
|
|
||
| operator: ConditionOperator | ||
| """Operator to be applied for this condition.""" | ||
| value: NotRequired[str | None] | ||
| """Value to be compared against in this condition. May be `None` for `IS_SET` and `IS_NOT_SET` operators.""" | ||
| property_: NotRequired[str | None] | ||
| """The property (context key) this condition applies to. May be `None` for the `PERCENTAGE_SPLIT` operator. | ||
|
|
||
| Named `property_` to avoid conflict with Python's `property` built-in. | ||
| """ | ||
|
|
||
|
|
||
| class SegmentRule(TypedDict): | ||
| """Represents a rule within a segment used by Flagsmith engine.""" | ||
|
|
||
| type: RuleType | ||
| """Type of the rule, defining how conditions are evaluated.""" | ||
| rules: "list[SegmentRule]" | ||
| """Nested rules within this rule.""" | ||
| conditions: list[SegmentCondition] | ||
| """Conditions that must be met for this rule, evaluated based on the rule type.""" | ||
|
|
||
|
|
||
| class Segment(TypedDict): | ||
| """Represents a Flagsmith segment. Carries rules, feature overrides, and segment rules.""" | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the segment in Core.""" | ||
| name: str | ||
| """Name of the segment.""" | ||
| rules: list[SegmentRule] | ||
| """List of rules within the segment.""" | ||
| feature_states: list[FeatureState] | ||
| """List of segment overrides.""" | ||
|
|
||
|
|
||
| class Organisation(TypedDict): | ||
| """Represents data about a Flagsmith organisation. Carries settings necessary for an SDK API operation.""" | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the organisation in Core.""" | ||
| name: str | ||
| """Organisation name as set via Core.""" | ||
| feature_analytics: NotRequired[bool] | ||
| """Whether the SDK API should log feature analytics events for this organisation. Defaults to `False`.""" | ||
| stop_serving_flags: NotRequired[bool] | ||
| """Whether flag serving is disabled for this organisation. Defaults to `False`.""" | ||
| persist_trait_data: NotRequired[bool] | ||
| """If set to `False`, trait data will never be persisted for this organisation. Defaults to `True`.""" | ||
|
|
||
|
|
||
| class Project(TypedDict): | ||
| """Represents data about a Flagsmith project. Carries settings necessary for an SDK API operation.""" | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the project in Core.""" | ||
| name: str | ||
| """Project name as set via Core.""" | ||
| organisation: Organisation | ||
| """The organisation that this project belongs to.""" | ||
| segments: list[Segment] | ||
| """List of segments.""" | ||
| server_key_only_feature_ids: NotRequired[list[DynamoInt]] | ||
| """List of feature IDs that are skipped when the SDK API serves flags for a public client-side key.""" | ||
| enable_realtime_updates: NotRequired[bool] | ||
| """Whether the SDK API should use real-time updates. Defaults to `False`. Not currently used neither by SDK APIs nor by SDKs themselves.""" | ||
| hide_disabled_flags: NotRequired[bool | None] | ||
| """Whether the SDK API should hide disabled flags for this project. Defaults to `False`.""" | ||
|
|
||
|
|
||
| class Integration(TypedDict): | ||
| """Represents evaluation integration data.""" | ||
|
|
||
| api_key: NotRequired[str | None] | ||
| """API key for the integration.""" | ||
| base_url: NotRequired[str | None] | ||
| """Base URL for the integration.""" | ||
|
|
||
|
|
||
| class DynatraceIntegration(Integration): | ||
| """Represents Dynatrace evaluation integration data.""" | ||
|
|
||
| entity_selector: str | ||
| """A Dynatrace entity selector string.""" | ||
|
|
||
|
|
||
| class Webhook(TypedDict): | ||
| """Represents a webhook configuration.""" | ||
|
|
||
| url: str | ||
| """Webhook target URL.""" | ||
| secret: str | ||
| """Secret used to sign webhook payloads.""" | ||
|
|
||
|
|
||
| class _EnvironmentFields(TypedDict): | ||
| """Common fields for Environment documents.""" | ||
|
|
||
| name: NotRequired[str] | ||
| """Environment name. Defaults to an empty string if not set.""" | ||
| updated_at: NotRequired[DateTimeStr | None] | ||
| """Last updated timestamp. If not set, current timestamp should be assumed.""" | ||
|
|
||
| project: Project | ||
| """Project-specific data for this environment.""" | ||
| feature_states: list[FeatureState] | ||
| """List of feature states representing the environment defaults.""" | ||
|
|
||
| allow_client_traits: NotRequired[bool] | ||
| """Whether the SDK API should allow clients to set traits for this environment. Identical to project-level's `persist_trait_data` setting. Defaults to `True`.""" | ||
| hide_sensitive_data: NotRequired[bool] | ||
| """Whether the SDK API should hide sensitive data for this environment. Defaults to `False`.""" | ||
| hide_disabled_flags: NotRequired[bool | None] | ||
| """Whether the SDK API should hide disabled flags for this environment. If `None`, the SDK API should fall back to project-level setting.""" | ||
| use_identity_composite_key_for_hashing: NotRequired[bool] | ||
| """Whether the SDK API should set `$.identity.key` in engine evaluation context to identity's composite key. Defaults to `False`.""" | ||
| use_identity_overrides_in_local_eval: NotRequired[bool] | ||
| """Whether the SDK API should return identity overrides as part of the environment document. Defaults to `False`.""" | ||
|
|
||
| amplitude_config: NotRequired[Integration | None] | ||
| """Amplitude integration configuration.""" | ||
| dynatrace_config: NotRequired[DynatraceIntegration | None] | ||
| """Dynatrace integration configuration.""" | ||
| heap_config: NotRequired[Integration | None] | ||
| """Heap integration configuration.""" | ||
| mixpanel_config: NotRequired[Integration | None] | ||
| """Mixpanel integration configuration.""" | ||
| rudderstack_config: NotRequired[Integration | None] | ||
| """RudderStack integration configuration.""" | ||
| segment_config: NotRequired[Integration | None] | ||
| """Segment integration configuration.""" | ||
| webhook_config: NotRequired[Webhook | None] | ||
| """Webhook configuration.""" | ||
|
|
||
|
|
||
| ### Root document schemas below. Indexed fields are marked as **INDEXED** in the docstrings. ### | ||
|
|
||
|
|
||
| class EnvironmentAPIKey(TypedDict): | ||
| """Represents a server-side API key for a Flagsmith environment. | ||
|
|
||
| **DynamoDB table**: `flagsmith_environment_api_key` | ||
| """ | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the environment API key in Core. **INDEXED**.""" | ||
| key: str | ||
| """The server-side API key string, e.g. `"ser.xxxxxxxxxxxxx"`. **INDEXED**.""" | ||
| created_at: DateTimeStr | ||
| """Creation timestamp.""" | ||
| name: str | ||
| """Name of the API key.""" | ||
| client_api_key: str | ||
| """The corresponding public client-side API key.""" | ||
| expires_at: NotRequired[DateTimeStr | None] | ||
| """Expiration timestamp. If `None`, the key does not expire.""" | ||
| active: bool | ||
| """Whether the key is active. Defaults to `True`.""" | ||
|
|
||
|
|
||
| class Identity(TypedDict): | ||
| """Represents a Flagsmith identity within an environment. Carries traits and feature overrides. | ||
|
|
||
| **DynamoDB table**: `flagsmith_identities` | ||
| """ | ||
|
|
||
| identifier: str | ||
| """Unique identifier for the identity. **INDEXED**.""" | ||
| environment_api_key: str | ||
| """API key of the environment this identity belongs to. Used to scope the identity within a specific environment. **INDEXED**.""" | ||
| identity_uuid: UUIDStr | ||
| """The UUID for this identity. **INDEXED**.""" | ||
| composite_key: str | ||
| """A composite key combining the environment and identifier. **INDEXED**. | ||
|
|
||
| Generated as: `{environment_api_key}_{identifier}`. | ||
| """ | ||
| created_date: DateTimeStr | ||
| """Creation timestamp.""" | ||
| identity_features: NotRequired[list[FeatureState]] | ||
| """List of identity overrides for this identity.""" | ||
| identity_traits: list[Trait] | ||
| """List of traits associated with this identity.""" | ||
| django_id: NotRequired[DynamoInt | None] | ||
| """Unique identifier for the identity in Core. If identity created via Core's `edge-identities` API, this can be missing or `None`.""" | ||
|
|
||
|
|
||
| class Environment(_EnvironmentFields): | ||
| """Represents a Flagsmith environment. Carries all necessary data for flag evaluation within the environment. | ||
|
|
||
| **DynamoDB table**: `flagsmith_environments` | ||
| """ | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the environment in Core. **INDEXED**.""" | ||
| api_key: str | ||
| """Public client-side API key for the environment. **INDEXED**.""" | ||
|
|
||
|
|
||
| class EnvironmentV2Meta(_EnvironmentFields): | ||
| """Represents a Flagsmith environment. Carries all necessary data for flag evaluation within the environment. | ||
|
|
||
| **DynamoDB table**: `flagsmith_environments_v2` | ||
| """ | ||
|
|
||
| environment_id: str | ||
| """Unique identifier for the environment in Core. **INDEXED**.""" | ||
| environment_api_key: str | ||
| """Public client-side API key for the environment. **INDEXED**.""" | ||
| document_key: Literal["_META"] | ||
| """The fixed document key for the environment v2 document. Always `"_META"`. **INDEXED**.""" | ||
|
|
||
| id: DynamoInt | ||
| """Unique identifier for the environment in Core. Exists for compatibility with the API environment document schema.""" | ||
|
|
||
|
|
||
| class EnvironmentV2IdentityOverride(TypedDict): | ||
| """Represents an identity override. | ||
|
|
||
| **DynamoDB table**: `flagsmith_environments_v2` | ||
| """ | ||
|
|
||
| environment_id: str | ||
| """Unique identifier for the environment in Core. **INDEXED**.""" | ||
| document_key: str | ||
| """The document key for this identity override, formatted as `identity_override:{feature Core ID}:{identity UUID}`. **INDEXED**.""" | ||
| environment_api_key: str | ||
| """Public client-side API key for the environment. **INDEXED**.""" | ||
| identifier: str | ||
| """Unique identifier for the identity. **INDEXED**.""" | ||
| identity_uuid: str | ||
| """The UUID for this identity, used by `edge-identities` APIs in Core. **INDEXED**.""" | ||
| feature_state: FeatureState | ||
| """The feature state override for this identity.""" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment density here is quite high. Many of these docstrings just restate what the field name already tells us (name: str with "Name of the feature", enabled: bool with "Whether the feature is enabled"). Could we trim these down to only the non-obvious stuff - constraints, edge cases, gotchas?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. I'd like to keep the docstrings because even obvious field descriptions help to remove ambiguity, and having all fields documented help to support the discipline to document new fields going forward.
If anything, I'd add a linter that makes sure every field has a docstring in this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna have to disagree here. If a field name needs a docstring to explain what it is, that's a sign the field name itself isn't good enough. The name and type should tell the story. Comments should explain why, not what.
The maintenance burden is real too - every field change now requires updating two places. Comments that just mirror the code tend to drift out of sync and become misleading over time.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes the why and the what are the same, and sometimes they're different. The users of the code shouldn't have to guess. This is what I meant by removing ambiguity.
It's hard to imagine this scenario for me, given that document schemas are supposed to be forward compatible. Once the field is added, it stays. If its purpose changes or it gets deprecated, we only have to edit the docstring. Ideally, I don't see us changing the field names at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s open the floor for a wider discussion.: @matthewelwell @Zaimwa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do tend to agree with Gagan here.
As an example, this comment seems unnecessary to me, as I think Gagan already mentioned.
I'm happy for us to lean on the overly verbose side here, but I think this example is probably a little much.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to weed out the docstrings on my own, but noticed I couldn't objectively separate the noise because in my mind, almost 100% of the documentation is signal.
Sorry to drag this on, but my suggestion is to have a short session later to sort this.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lean towards @khvn26's stance here.
AlthoughI agree fields should be self-explanatory, it isn't always the case (especially for legacy fields, and I don't see a near future in which we will change that).
To me, the maintenance burden would actually increase if we need to decide on a per-case basis because it might bring too much subjectivity:
Some descriptions are repetitive (cf enabled), others might be really helpful when lacking context (cf composite_key) and i'd rather have too much information available than not enough.
Also, since this is a public repo, I'd prefer assume that external readers will not have the same level of context that we have, which leads me to be overly cautious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth noting that signal/noise ratio will become even more subjective as we continue to grow the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flagsmith-common/src/flagsmith_models/__init__.py
Lines 153 to 163 in 4d8c83a
In the above example, most fields sound self-explanatory. Except for
feature_states, which inherits its name from the data model rather than from the user experience, thus needs a comment to explain what but differently. This is yet another category amongwhatandwhy.Now if only this field has a human-readable description, and not the others, we create subjectiveness, and we lose structure — this connects with @Zaimwa9's comment. It looks like the question is whether we need to adhere to a structure. I think we do.
P.S. I do disagree with docstrings after code. 😞