-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(api): custom queries support for rds using sql directive #1816
feat(api): custom queries support for rds using sql directive #1816
Conversation
); | ||
} | ||
|
||
const statement = config.statement ?? (config.reference ? context.customQueries.get(config.reference) : MISSING_QUERY_STATEMENT); |
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.
Instead of falling back to a MISSING_QUERY_STATEMENT
and proceeding with resolver generation, shouldn't the case of not having valid statement
or a pre-loaded reference
SQL file be an Error instead? If you already have those checks, is MISSING_QUERY_STATEMENT
default needed 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.
MISSING_QUERY_STATEMENT has been added to make the compiler happy. Otherwise statement
would be string | undefined
instead of a string
. I could have used context.customQueries.get(config.reference)!
but preferred this approach better.
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.
There are couple of cases that we should check:
- When the
statements
here is an empty string. - When both statements and reference are provided.
set(ref('lambdaInput.operation'), str(operation)), | ||
set(ref('lambdaInput.operationName'), str(operationName)), | ||
set(ref('lambdaInput.parameters'), methodCall(ref('util.defaultIfNull'), ref('context.arguments'), obj({}))), | ||
obj({ |
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.
Is there a clean way to detect and fail fast if the variables expected by the SQL statement do not exist in the input? This will save customers invalid lambda invocations.
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.
No, it is not in scope of this PR. I'm not sure whether we should do that. For example, if customer wants to do additional data transformation to input and override the resolver, they can do that now. If we add that additional validation, this usecase would be impacted.
const stack: cdk.Stack = context.stackManager.createStack(SQL_DIRECTIVE_STACK); | ||
const env = context.synthParameters.amplifyEnvironmentName; | ||
|
||
stack.templateOptions.templateFormatVersion = '2010-09-09'; |
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.
Why is this required?
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.
This is to handle an edge case where customer hasn't used any @model
types but using the new SQL directive alone. This makes sure that the stack is created.
708963c
into
aws-amplify:feature/rds-support-preview2
Description of changes
Add custom queries support for RDS using @Sql directive.
@sql(statement: "xyx")
allows to define SQL queries inline in RDS schema file.@sql(reference: "abc")
allows to define SQL statements in a file under api/sql-statements directory.CDK / CloudFormation Parameters Changed
NA
Issue #, if available
NA
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.