fix(appsync):appsync.HttpDataSourceProps erroneously extends BaseDataSourceProps

[ScottRobinson03]

Reason for this change

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

Description of changes

Makes the HttpDataSourceProps type extend BackedDataSourceProps, instead of BaseDataSourceProps.

Description of how you validated changes

N/A

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 7d3051a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[ScottRobinson03]

Exemption Request

This PR is just fixing a type, so updating tests isn't appropriate.

[TheRealAmazonKendra]

This PR doesn't give me any reason to believe that the choice to only update on set of props wasn't intentional.

Given that it was a change made in 2020 and there are no linked issues on this PR, we need more context on why this change is necessary.

[ScottRobinson03]

This PR doesn't give me any reason to believe that the choice to only update on set of props wasn't intentional.

Given that it was a change made in 2020 and there are no linked issues on this PR, we need more context on why this change is necessary.

@TheRealAmazonKendra I've already given an explanation in the PR description. There's not really much to explain. The type currently extends BaseDataSource, but it should extend BackedDataSource instead (to match what the HTTPDataSource class extends, as of #11185).

It's needed because the types are currently wrong and therefore autocomplete, compilation, etc. is affected.

[TheRealAmazonKendra]

Which props are present/missing that shouldn't be? Is there an open issue for this? Updating this will likely be a breaking change.

[ScottRobinson03]

Which props are present/missing that shouldn't be? Is there an open issue for this? Updating this will likely be a breaking change.

You can look at the types yourself to see this. The only difference is that BackedDataSource includes the serviceRole prop, which BaseDataSource doesn't. See the source code. It's not a breaking change since there's no keys removed, just one key added. To be clear, in an ideal world this PR wouldn't be necessary, and this would've been noticed in #11185 some 3½ ish years ago. This PR is SOLELY to fix a mistake from that PR that wasn't noticed.

From what I found, there's not an open issue, hence I didn't link one to this PR. Such a tiny change that's obviously correct also shouldn't need an issue, hence I didn't create one -- there's nothing to discuss here, the type is just blatantly wrong and this PR fixes it.

[TheRealAmazonKendra]

Which props are present/missing that shouldn't be? Is there an open issue for this? Updating this will likely be a breaking change.

You can look at the types yourself to see this. The only difference is that BackedDataSource includes the serviceRole prop, which BaseDataSource doesn't. See the source code. It's not a breaking change since there's no keys removed, just one key added. To be clear, in an ideal world this PR wouldn't be necessary, and this would've been noticed in #11185 some 3½ ish years ago. This PR is SOLELY to fix a mistake from that PR that wasn't noticed.

From what I found, there's not an open issue, hence I didn't link one to this PR. Such a tiny change that's obviously correct also shouldn't need an issue, hence I didn't create one -- there's nothing to discuss here, the type is just blatantly wrong and this PR fixes it.

Please review the Pull Request section of our Contributing Guide.

Your pull request should contain all the information we need to assess the change. From our contributing guide:

- Pull request body should describe motivation. Think about your code reviewers and what information they need in order to understand what you did. If it's a big commit (hopefully not), try to provide some good entry points so it will be easier to follow.
  - For bugs, describe bug, root cause, solution, potential alternatives considered but discarded.
  - For features, describe use case, most salient design aspects (especially if new), potential alternatives.

Adding properties can, in fact, be a breaking change. If we are adding a mandatory prop that is breaking. From the information provided here, I can't tell which props specifically are being removed/added and if the props you are removing/adding are required.

Your explanation of this change should include that information.

Also from our contributing guide:

- If not obvious (i.e. from unit tests), describe how you verified that your change works.

In order to accept this change, we will need to have tests. This change would change the contract for this construct so it would require unit tests and integ tests. From the contributing guide regarding integ tests:

#### When are integration tests required?

The following list contains common scenarios where we know that integration tests are required. This is not an exhaustive list and we will, by default, require integration tests for all new features and all fixes unless there is a good reason why one is not needed.

1. Adding a new feature
2. Adding a fix to an existing feature
3. Involves configuring resource types across services (i.e. integrations)
4. Adding a new supported version (e.g. a new [AuroraMysqlEngineVersion](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds.AuroraMysqlEngineVersion.html))
5. Adding any functionality via a [Custom Resource](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources-readme.html)

Lastly, as this is a fix, please note this section of the contributing guide regarding PR titles:

- Titles for feat and fix PRs end up in the change log. Think about what makes most sense for users reading the changelog while writing them.
  - feat: describe the feature (not the action of creating the commit or PR, for example, avoid words like "added" or "changed")
  - fix: describe the bug (not the solution)

[ScottRobinson03]

Your pull request should contain all the information we need to assess the change. From our contributing guide:

- Pull request body should describe motivation. Think about your code reviewers and what information they need in order to understand what you did. If it's a big commit (hopefully not), try to provide some good entry points so it will be easier to follow.
  - For bugs, describe bug, root cause, solution, potential alternatives considered but discarded.

I included these. The bug, cause, and solution are all in the PR description. Alternatives isn't applicable since there's only one possible sensible fix (the one implemented).

I can't tell which props specifically are being removed/added and if the props you are removing/adding are required.

As I said in my previous message the ONLY difference is the ADDITION of the serviceRole prop, which as shown in the linked source code is optional. Adding an optional prop is not a breaking change.

In order to accept this change, we will need to have tests. This change would change the contract for this construct so it would require unit tests and integ tests. From the contributing guide regarding integ tests:

This is changing a type. Not runtime code. What has this got to do with unit and integration tests?

Lastly, as this is a fix, please note this section of the contributing guide regarding PR titles:

- Titles for feat and fix PRs end up in the change log. Think about what makes most sense for users reading the changelog while writing them.
  - fix: describe the bug (not the solution)

Have updated the title to be clearer.

[ScottRobinson03]

@TheRealAmazonKendra I'd like to see this PR get merged, so would you please expand on what exactly you're eluding to by your comment around unit and integration tests.

Even after looking into the pre-existing unit and integration tests, I still don't understand how updating a type requires updating tests that ultimately check runtime behaviour.

No runtime logic is changed in this PR, so there's no new functionality for me to update the tests to check against. I suppose you could argue that editor autocomplete is an added functionality in this PR but I don't see how you can unit/integration test this.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[ScottRobinson03]

@TheRealAmazonKendra bump

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.