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

appsync: appsync.HttpDataSourceProps erroneously extends BaseDataSourceProps #31979

Closed
1 task
ScottRobinson03 opened this issue Nov 1, 2024 · 8 comments · Fixed by #32065
Closed
1 task
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@ScottRobinson03
Copy link
Contributor

ScottRobinson03 commented Nov 1, 2024

Describe the bug

In #11185, the HttpDataSource class was updated to extend BackedDataSource instead of BaseDataSource, however the HttpDataSourceProps type wasn't updated to reflect this change.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The HttpDataSourceProps type should extend BackendDataSourceProps

Current Behavior

The HttpDataSourceProps type erroneously extends BaseDataSourceProps

Reproduction Steps

Minimum-repro to see the type error:

import * as appsync from "aws-cdk-lib/aws-appsync";
import type { IRole } from "aws-cdk-lib/aws-iam";

declare const myApi: appsync.GraphqlApi;
declare const serviceRole: IRole;

const stepFunctionHttpDataSource = new appsync.HttpDataSource(
    myApi,
    "MyStepFunctionHTTPDataSource",
    {
        api: myApi,
        authorizationConfig: {
            signingRegion: "eu-west-1",
            signingServiceName: "states",
        },
        endpoint: `https://states.eu-west-1.amazonaws.com`,
        name: "StepFunctionHTTPDataSource",
        // Providing the `serviceRole` incorrectly throws a type error
        serviceRole,
    }
);

image

Possible Solution

Update the HttpDataSourceProps type to extend BackendDataSourceProps

Additional Information/Context

The only difference between BaseDataSourceProps and BackedDataSourceProps, is that BackedDataSourceProps adds an optional prop serviceRole, which is required for workflows that require custom permissions, such as the above example of starting a state machine.

The code for BaseDataSourceProps and BackedDataSourceProps is here, and shown below:

/**
 * Base properties for an AppSync datasource
 */
export interface BaseDataSourceProps {
  /**
   * The API to attach this data source to
   */
  readonly api: IGraphqlApi;
  /**
   * The name of the data source
   *
   * @default - id of data source
   */
  readonly name?: string;
  /**
   * the description of the data source
   *
   * @default - None
   */
  readonly description?: string;
}

/**
 * properties for an AppSync datasource backed by a resource
 */
export interface BackedDataSourceProps extends BaseDataSourceProps {
  /**
   * The IAM service role to be assumed by AppSync to interact with the data source
   *
   * @default -  Create a new role
   */
  readonly serviceRole?: IRole;
}

CDK CLI Version

2.139.0

Framework Version

No response

Node.js Version

20

OS

MacOS Sonoma 14.7

Language

TypeScript

Language Version

TypeScript 5.6.3

Other information

NB: #29689 would have fixed this however the PR was closed after going stale when requesting an exemption to requiring tests.

@ScottRobinson03 ScottRobinson03 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 1, 2024
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Nov 1, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Nov 4, 2024
@khushail khushail self-assigned this Nov 4, 2024
@khushail khushail added the needs-reproduction This issue needs reproduction. label Nov 4, 2024
@khushail
Copy link
Contributor

khushail commented Nov 6, 2024

@ScottRobinson03 , thanks for opening the issue and providing the code links.
Yes, this observation totally makes sense and I can confirm that the error seen while trying to declare the serviceRole -

Screenshot 2024-11-05 at 6 51 02 PM

As its already pointed out, HttpDataSourceProps extends BaseDataSourceProps

export interface HttpDataSourceProps extends BaseDataSourceProps {

and BaseDataSourceProps has these attrtibutes-

https://github.com/aws/aws-cdk/blob/a21a0b1392faecbb0a19a6ca90b64a1da8ad5d81/packages/aws-cdk-lib/aws-appsync/lib/data-source.ts#L19C1-L36C2

export interface BaseDataSourceProps {
  /**
   * The API to attach this data source to
   */
  readonly api: IGraphqlApi;
  /**
   * The name of the data source
   *
   * @default - id of data source
   */
  readonly name?: string;
  /**
   * the description of the data source
   *
   * @default - None
   */
  readonly description?: string;
}

and HttpDataSource extends BackendDataSource-

https://github.com/aws/aws-cdk/blob/00f70f1976adf6539d909fa0a5cf583c5b5289ac/packages/aws-cdk-lib/aws-appsync/lib/data-source.ts#L276C14-L276C28

where the serviceRole is provided by BackedDataSourceProps-

export interface BackedDataSourceProps extends BaseDataSourceProps {

export interface BackedDataSourceProps extends BaseDataSourceProps {
  /**
   * The IAM service role to be assumed by AppSync to interact with the data source
   *
   * @default -  Create a new role
   */
  readonly serviceRole?: IRole;
}

which is the reason why the error is being seen.

@khushail khushail added effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Nov 6, 2024
@khushail khushail removed their assignment Nov 6, 2024
@ScottRobinson03
Copy link
Contributor Author

Thanks for looking into this @khushail.

What's the next step to get this fixed? Can #29689 be re-opened, or would I need to create a new PR?

@khushail
Copy link
Contributor

khushail commented Nov 7, 2024

@ScottRobinson03 , I think you should be able to reopen the PR. Also please link this issue in the description and update whatever is needed to get the community review done.

Let me know if you need any further help! Appreciate your contribution.

@ScottRobinson03
Copy link
Contributor Author

ScottRobinson03 commented Nov 7, 2024

@khushail Thanks, I've updated the PR description accordingly and left a comment too, however the comment didn't reopen the PR as I suspected. I assume this means I don't have permissions to reopen the PR, so are you perhaps able to do this on my behalf?

@khushail
Copy link
Contributor

khushail commented Nov 7, 2024

@ScottRobinson03 , its reopened. Please feel free to work on it now.

@ScottRobinson03
Copy link
Contributor Author

Thanks @khushail. Afaik there's no more work to do from me (the PR is just a one-line fix), so it's just pending reviews (and an integ-test exemption approval, since it's just a type fix [there's no runtime code to actually test]).

@ScottRobinson03
Copy link
Contributor Author

#29689 got automatically closed again, so I've replaced it with #32065.

@mergify mergify bot closed this as completed in #32065 Dec 5, 2024
mergify bot pushed a commit that referenced this issue Dec 5, 2024
…DataSourceProps` (#32065)

### Reason for this change
Closes #31979.
Replaces #29689.

In #11185, the `HttpDataSource` class was updated to extend `BackedDataSource` instead of `BaseDataSource`, however the `HttpDataSourceProps` type wasn't updated to reflect this change. This PR makes the `HttpDataSourceProps` type reflect the change.

### Description of changes
Makes the `HttpDataSourceProps` type extend `BackedDataSourceProps`, instead of `BaseDataSourceProps`. This means users are able to provide the `serviceRole` prop without getting a type error.

### Description of how you validated changes
The below code snippet no longer gives an incorrect type error when providing the serviceRole, as it did before:
```ts
import * as appsync from "aws-cdk-lib/aws-appsync";
import type { IRole } from "aws-cdk-lib/aws-iam";

declare const myApi: appsync.GraphqlApi;
declare const serviceRole: IRole;

const stepFunctionHttpDataSource = new appsync.HttpDataSource(
    myApi,
    "MyStepFunctionHTTPDataSource",
    {
        api: myApi,
        authorizationConfig: {
            signingRegion: "eu-west-1",
            signingServiceName: "states",
        },
        endpoint: `https://states.eu-west-1.amazonaws.com`,
        name: "StepFunctionHTTPDataSource",
        // Providing the `serviceRole` now correctly DOESN'T throw a type error
        serviceRole,
    }
);
```
Copy link

github-actions bot commented Dec 5, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
2 participants