Skip to content

feat: add AttributeProvider #83

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yuzawa-san
Copy link
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

This is an expansion upon my prior work in #74

Describe the solution you've provided

I added a new interface (AttributeProvider) and API (ContextBuilder.attributes(AttributeProvider)) which allows the developer to pass in a dynamic and lazy set of attributes. Once again, there are decent number of attributes which are available across our shared LDContexts but only a few attributes are actually used in most flags. This is a way to flexibly provide these attributes, while keeping the copy on write semantics introduced in my prior PR.

I kept the original PR's work, but renamed AttributeMap to Attributes (safe since none of it is public). I made Attributes abstract and have two implementations: one for maps (use the ContextBuilder.set() methods) and one for AttributeProvider). As seen in my new unit test case, it is possible to mix and match.

Describe alternatives you've considered

My goal is to basically to something like this in my code:

private static final AttributeProviderFactory PROVIDER_FACTORY = AttributeProviderFactory.of(MyPojo.class).set("foo", MyPojo::getFoo).set("bar", MyPojo::getBar).build();

void myMethod() {
MyPojo myPojo = .....
LDContext.builder("key").attributes(PROVIDER_FACTORY.wrap(myPojo)).build()
}

I considered adding AttributeProviderFactory, but I settled on just adding the AttributeProvider for now. I did not want the library to get too stuck to our use case, also the jdk 7 min version is troublesome to add lambda support like that.

Additional context

See flame chart analysis from prior PR. This PR would ideally get all HashMap things eliminated since the LDValues would be provided on-demand.

@yuzawa-san yuzawa-san requested a review from a team as a code owner July 14, 2025 17:16
@tanderson-ld
Copy link
Contributor

Hi again @yuzawa-san, I'll discuss these changes with the team within the next week and will let you know our thoughts. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants