Skip to content
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

AppConfig "empty" response handling #1672

Open
chrisclayson opened this issue Jun 26, 2024 · 5 comments
Open

AppConfig "empty" response handling #1672

chrisclayson opened this issue Jun 26, 2024 · 5 comments
Labels
bug Something isn't working status/staged-next-release

Comments

@chrisclayson
Copy link
Contributor

chrisclayson commented Jun 26, 2024

What were you trying to accomplish?

Using the AppConfigProvider for the parameters project, I noticed a bug in the handling of instances where AppConfig returns the empty response (i.e. where config has not changed).

This was specifically for a "Freeform configuration profile" rather than feature flags (which I have not tested).

Expected Behavior

The provider should re-use the cached config.

Current Behavior

The provider returns an empty string for the config rather than the cached config. The seems to be because the provider only checks for null and not "empty".

Possible Solution

I implemented a copy of the AppConfigProvider class and added a check for response.configuration().asByteArray().length > 0 and this worked, i.e.:

        // Get the value of the key. Note that AppConfig will return null if the value
        // has not changed since we last asked for it in this session - in this case
        // we return the value we stashed at last request.
        String value = response.configuration() != null && response.configuration().asByteArray().length > 0 ?
                response.configuration().asUtf8String() : // if we have a new value, use it
                establishedSession != null ?
                        establishedSession.lastConfigurationValue :
                        // if we don't but we have a previous value, use that
                        null; // otherwise we've got no value

        // Update the cache so we can get the next value later
        establishedSessions.put(key, new EstablishedSession(nextSessionToken, value));

        return value;

I'm happy to submit as a PR.

Steps to Reproduce (for bugs)

  • Setup a freeform configuration profile in AppConfig with some value (we used a JSON object { "version": 1 })
  • Retrieve from a lambda using PowerTools parameters:
          var config = ParamManager.getAppConfigProvider("test", "test")
                .withMaxAge(30, ChronoUnit.SECONDS)
                .get("test");
  • Fire continuous traffic at the lambda

The first retrieval, on any given runtime environment, will work fine, but after the 30 second TTL the next retrieve from AppConfg returns an empty string, which is returned from the get() method rather than original (cached value).

Environment

  • Powertools for AWS Lambda (Java) 1.18.0
  • Built with Gradle
  • Java 17 runtime
@jeromevdl
Copy link
Contributor

Thank you @chrisclayson for submitting this. And you're more than welcome to create a PR.

@scottgerring, can you have a look?

@chrisclayson
Copy link
Contributor Author

I'll have to fork to submit PR, so I'll give that a go.

One thing I forgot to mention is, initially, this manifest itself with this error here ... I haven't fully worked out why, but we use a transformer, and I don't think it was handling the empty (or null) config being passed to it very well but just confused as to why it triggered that particular error.

I'll raise as a separate bug if I think it one for you to worry about ... it may have been my ropey transformer code.

@chrisclayson
Copy link
Contributor Author

chrisclayson commented Aug 1, 2024

Wondering if there is any plans to either fix the issue or merge my PR in (I note it failed a bunch of checks, which I'm assuming because from a fork)?

I have a use case for this and, whilst I can supply my own provider with the fix, I'd rather use a supported "out of the box" version ;-)

@scottgerring
Copy link
Contributor

@jeromevdl @chrisclayson apologies for the slow response!
Fix looks about right to me - maybe something like StringUtils.isNullOrEmpty is convenient?

@chrisclayson
Copy link
Contributor Author

Afternoon, I see this has been tagged for the next release ... just wondering if there is a timeline for the release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status/staged-next-release
Projects
Status: Coming soon
Development

No branches or pull requests

3 participants