feat: Update in-process resolver to support flag metadata #1102 #1122
Conversation
…e#1102 Signed-off-by: christian.lutnik <[email protected]>
github-actions
bot
assigned ajhelsby, beeme1mr, Kavindu-Dodan, matthewelwell, thisthat and toddbaert
github-actions
bot
requested review from
ajhelsby,
beeme1mr,
Kavindu-Dodan,
matthewelwell,
thisthat and
toddbaert
| return fallBackMetadata; | ||
| } | ||
|
|
||
| ImmutableMetadata.ImmutableMetadataBuilder metadataBuilder = ImmutableMetadata.builder(); |
There was a problem hiding this comment.
I would prefer to have a mergeMetdata or addMetadata method in the ImmutableMetadataBuilder
| @@ -79,7 +81,7 @@ public void eventHandling() throws Throwable { | |||
| final MutableStructure syncMetadata = new MutableStructure(); | |||
| syncMetadata.add(key, val); | |||
|
|
|||
| InProcessResolver inProcessResolver = getInProcessResolverWth(new MockStorage(new HashMap<>(), sender), | |||
| InProcessResolver inProcessResolver = getInProcessResolverWith(new MockStorage(new HashMap<>(), sender), | |||
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
toddbaert
force-pushed
the
feat/Update-in-process-resolver-to-support-flag-metadata
branch
3 times, most recently
from
1fa47fc to
1285149
Compare
…e#1102 Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
feat/Update-in-process-resolver-to-support-flag-metadata
branch
from
1285149 to
c08e7bc
Compare
|
@chrfwow this is definitely a step in the right direction - I think there's probably some null check missing related to the metadata causing the e2e failures: Working with @beeme1mr and a few others in in slack, we've decided another thing we'd want here is a very basic mechanism for reducing duplication: the ability to add metadata for the entire flag set at the "top level" which all flags inherit: {
"metadata": {
"foo": "bar"
},
"flags": {
"flag1": {
// ...
"metadata": {
// "foo": "bar" is automatically included, but can be overridden if defined in `flag1`
}
}
}
}The same thing is implemented here: open-feature/js-sdk-contrib#1120 cc @beeme1mr |
…to-support-flag-metadata' into feat/Update-in-process-resolver-to-support-flag-metadata # Conflicts: # providers/flagd/schemas
| @JsonProperty("defaultVariant") String defaultVariant, | ||
| @JsonProperty("variants") Map<String, Object> variants, | ||
| @JsonProperty("targeting") @JsonDeserialize(using = StringSerializer.class) String targeting, | ||
| @JsonProperty("metadata") Map<String, Object> metadata |
There was a problem hiding this comment.
[question] Currently, we do need to provide an empty map everywhere to be backward compatible. Could we also omit this parameter and make it optional? more a question out of curiosity ;)
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
| assertThrows(IllegalArgumentException.class, () -> { | ||
| FlagParser.parseString(getFlagsFromResource(INVALID_FLAG), true); | ||
| }); | ||
| void validJsonConfigurationWithGlobalMetadataParsing() throws IOException { |
There was a problem hiding this comment.
Tests look good, thanks; I think I see all obvious the cases tested.
| @@ -178,6 +183,7 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC | |||
| return ProviderEvaluation.<T>builder() | |||
| .errorMessage("flag: " + key + " not found") | |||
| .errorCode(ErrorCode.FLAG_NOT_FOUND) | |||
| .flagMetadata(fallBackMetadata) | |||
There was a problem hiding this comment.
As a corollary to the inheritance of the flag set metadata, for error cases we should make a "best effort" to return whatever metadata we can - so here we should still return the flag set metadata. We've added this to the spec: https://github.com/open-feature/flagd/pull/1505/files#diff-28ba4b4041b6c39c4b9deb542276312bebdd08f45b39b29d860849d9e8b81a6eR297
The simplest way might be to store the flag set metadata on the flag store and expose it there.
This will greatly help in telemetry and debugging, since we'll have some idea of the flag set, it's version, etc in error reporting and metrics.
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
…to-support-flag-metadata' into feat/Update-in-process-resolver-to-support-flag-metadata # Conflicts: # providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/InProcessResolver.java
…-feature#1102 Signed-off-by: christian.lutnik <[email protected]>
This PR
Adds support for flag metadata in the in-process resolver
Related Issues
Fixes #1102