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

fix: Lambda client env var name issue #2324

Merged
merged 13 commits into from
Dec 17, 2024
Merged

Conversation

stocaaro
Copy link
Member

@stocaaro stocaaro commented Dec 12, 2024

Problem

When using the getAmplifyDataClientConfig function with a named dataBackend, the SSM variable names don't line up.

Example:

export const data = defineData({
  name: 'CoolName',
  schema,
  ...
})

with

// amplify/auth/post-confirmation/handler.ts
...
import { getAmplifyDataClientConfig } from '@aws-amplify/backend/function/runtime';
...

Results in a typescript error caused by mismatched env var names.

Issue number, if available:
#2320

Changes

  • Use AmplifyData as a prefix (which is included even if a user defined name is provided
  • Always pass the name and prefix through the AMPLIFY_DATA_DEFAULT_NAME SSM variable
  • getAmplifyDataClientConfig now uses the default name variable to use the correct env variables
  • Move toScreamingSnakeCase into the export surface of '@aws-amplify/platform-core' as SSM variables are normalized by this method and this normalization needs to be applied to consume these variables in getAmplifyDataClientConfig

Validation

  • Modified integ test to use defineData({name: '', ...})
  • Add unit tests to cover the named case
  • Manual validation

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: c14ab2c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@aws-amplify/backend-function Minor
@aws-amplify/backend-data Patch
@aws-amplify/platform-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@stocaaro stocaaro marked this pull request as ready for review December 16, 2024 13:48
@stocaaro stocaaro requested review from a team as code owners December 16, 2024 13:48
@stocaaro stocaaro added the run-e2e Label that will include e2e tests in PR checks workflow label Dec 16, 2024
.changeset/warm-sloths-tickle.md Outdated Show resolved Hide resolved
.changeset/warm-sloths-tickle.md Outdated Show resolved Hide resolved
modelIntrospectionSchemaKey,
['AMPLIFY_DATA_DEFAULT_NAME']: `${namePrefix}${this.name}`,
// @deprecated
[`${this.name}_GRAPHQL_ENDPOINT`]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this not create a duplicate when namePrefix is empty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about the same thing. Up on line 311, name prefix is left blank if the name is defaulted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not expected, we should avoid adding it. Also wanted to ask what's special about GRAPHQL_ENDPOINT that this behavior is not replicated for the other _MODEL_INTROSPECTION_SCHEMA_BUCKET_NAME and _MODEL_INTROSPECTION_SCHEMA_KEY

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GRAPHQL_ENDPOINT predates the MIS fields and customers may be depending on it directly. Since the other two are newer, @sobolk advised that they can be modified without concern for breaking change.

Also GRAPHQL_ENDPOINT is included in the prefixing, however, we are leaving an unprefixed version in place with a @Deprecation flag to enable removal with the next major version change.

@@ -307,14 +308,32 @@ class DataGenerator implements ConstructContainerEntryGenerator {

convertJsResolverDefinition(scope, amplifyApi, schemasJsFunctions);

const namePrefix = this.name === defaultName ? '' : defaultName;

const ssmEnvironmentScopeContext = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const ssmEnvironmentScopeContext = {
const ssmEnvironmentEntries = {

perhaps. this is a nit, feel free to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming came from the function that receives these entries/vars ssmEnvironmentEntriesGenerator.generateSsmEnvironmentEntries(scopeContext). I'm going to proceed with this naming and can revisit it in another change if we decide its clearer with different naming.

['AMPLIFY_DATA_DEFAULT_NAME']: `${namePrefix}${this.name}`,
};

const backwardsCompatibleScopeContext =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update this plz if you take above nit.

@stocaaro stocaaro merged commit f193105 into main Dec 17, 2024
84 of 86 checks passed
@stocaaro stocaaro deleted the stocaaro/lambda-client-fix/main branch December 17, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e Label that will include e2e tests in PR checks workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants