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

Implement SupportsDeferredBindingAttribute #1275

Merged
merged 3 commits into from Jan 18, 2023

Conversation

liliankasem
Copy link
Member

Issue describing the changes in this PR

resolves #1266

Today we have have a single SupportsDeferredBinding attribute that extensions can set to indicate that ParameterBindingData can be used (i.e. the extension supports sdk-type bindings). This is used by the FunctionMetadataGenerator to set the "supportsDeferringBinding" property for every function.

The goal for this issue is to refactor the SupportsDeferredBinding attribute so that instead of being a blanket "all or nothing" approach for extensions, we can set the supportsDeferredBinding flag per binding instead. For example, the CosmosDBTrigger does not support deferred binding but we can use deferred binding for the input binding.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

@liliankasem I am concerned this is both a public API breaking change as well as behavior breaking change. Do you have more context on if this is okay to perform now or will it need to wait until next version?

Or can we support both ways for now, and remove the legacy way later?

sdk/Sdk/FunctionMetadataGenerator.cs Show resolved Hide resolved
@surgupta-msft
Copy link
Contributor

surgupta-msft commented Jan 17, 2023

Can we add some tests for these changes? One place could be - FunctionMetadataGeneratorTests and a sample app/function with the attribute SupportsDeferredBinding

@liliankasem
Copy link
Member Author

liliankasem commented Jan 17, 2023

Can we add some tests for these changes? One place could be - FunctionMetadataGeneratorTests and a sample app/function with the attribute SupportsDeferredBinding

I don't believe we need a sample app for it, this attribute goes onto the worker extension input/output/trigger attribute classes not on customer function classes e.g.:

    [SupportsDeferredBinding]
    public sealed class CosmosDBInputAttribute : InputBindingAttribute
    {

For testing, this functionality is already covered with the FunctionMetadataGeneratorTests as they test the public entry all the way, all the other methods are private and will be tested via the GenerateFunctionMetadata method. We know things are working as expected as the properties dictionary is empty (as it should be since no extensions has that attribute yet). As we use this attribute, I expect the tests to break and we will need to update them check that properties dict is not empty when a bindings supports deferred binding (we're already doing this in the blob PR!)

@liliankasem
Copy link
Member Author

/check-enforcer evaluate

@liliankasem
Copy link
Member Author

/check-enforcer evaluate

@liliankasem
Copy link
Member Author

/check-enforcer reset

@liliankasem
Copy link
Member Author

/check-enforcer override

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor how we determine which extensions support deferred binding
4 participants