Skip to content

Implement resource metadata attribute #170

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.

Sign up for GitHub

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

Jump to bottom
Open
liliankasem wants to merge 5 commits into
base: feature/resources
Choose a base branch
from
Open

Implement resource metadata attribute #170

liliankasem wants to merge 5 commits into from

Conversation

@liliankasem
Copy link
Member

@liliankasem liliankasem commented Dec 17, 2025 •
edited
Loading

Issue describing the changes in this PR

resolves #161, #162

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
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@liliankasem liliankasem added the area: host-extension Items impacting the host extension label Dec 17, 2025
@liliankasem liliankasem force-pushed the liliankasem/resource-host-trigger branch from 3b0787c to 36a375a Compare December 19, 2025 01:06
@liliankasem liliankasem force-pushed the liliankasem/resource-metadata-attribute branch from a14d3f5 to 4be8f99 Compare December 19, 2025 01:07
@liliankasem liliankasem force-pushed the liliankasem/resource-host-trigger branch from 36a375a to ba01930 Compare December 19, 2025 01:12
@liliankasem liliankasem force-pushed the liliankasem/resource-metadata-attribute branch from 4be8f99 to 992e620 Compare December 19, 2025 01:15
@liliankasem liliankasem force-pushed the liliankasem/resource-host-trigger branch from ba01930 to d4203cc Compare December 19, 2025 19:31
@liliankasem liliankasem force-pushed the liliankasem/resource-metadata-attribute branch from 992e620 to a87d5ca Compare January 21, 2026 21:56
@liliankasem liliankasem marked this pull request as ready for review January 21, 2026 21:56
Base automatically changed from liliankasem/resource-host-trigger to feature/resources January 22, 2026 00:18
@liliankasem liliankasem force-pushed the liliankasem/resource-metadata-attribute branch from a87d5ca to cdf8f0a Compare January 22, 2026 00:32
@liliankasem liliankasem force-pushed the liliankasem/resource-metadata-attribute branch from cdf8f0a to 61c0f17 Compare January 22, 2026 00:34
fabiocav
fabiocav reviewed Jan 23, 2026
View reviewed changes
Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

The worker surface to support this should be relatively small, right? Can we combine that in the current PR?

src/Microsoft.Azure.Functions.Extensions.Mcp/Trigger/McpResourceTriggerBinding.cs Outdated
// First check if metadata was serialized from worker model
if (attribute.Metadata is { } metadataString)
{
var metadataList = JsonSerializer.Deserialize<List<KeyValuePair<string, object?>>>(metadataString, McpJsonSerializerOptions.DefaultOptions);
Copy link
Member

@fabiocav fabiocav Jan 23, 2026

Choose a reason for hiding this comment

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

Deserialization here if working with <string, object?> may also require special handling. That will be solved if moving to <string, string?>

Copy link
Member Author

@liliankasem liliankasem Jan 23, 2026

Choose a reason for hiding this comment

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

I'll investigate the special handling, but I don't think we should do string, string for the reason mentioned above

@liliankasem
Copy link
Member Author

liliankasem commented Jan 23, 2026

The worker surface to support this should be relatively small, right? Can we combine that in the current PR?

Yup it's not large, I can include it here

@liliankasem liliankasem added the area: dotnet-worker Items impacting the .NET worker label Jan 24, 2026
@liliankasem
Copy link
Member Author

liliankasem commented Jan 24, 2026

The worker surface to support this should be relatively small, right? Can we combine that in the current PR?

Yup it's not large, I can include it here

@fabiocav added the worker side too, but note that I had to add some stuff that is going to be merged in with Aishwarya's PR (like the trigger attrribute). I'll rebase once that is merged.

liliankasem
liliankasem commented Jan 24, 2026
View reviewed changes
src/Microsoft.Azure.Functions.Extensions.Mcp/Trigger/ResourceReturnValueBinder.cs
}

// If metadata is not set in content, use attribute metadata
// Q: Do we want to implement merge behaviour instead?
Copy link
Member Author

@liliankasem liliankasem Jan 24, 2026

Choose a reason for hiding this comment

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

Question here. I think its fine as is

liliankasem
liliankasem commented Jan 24, 2026
View reviewed changes
src/Microsoft.Azure.Functions.Worker.Extensions.Mcp/McpResourceTriggerAttribute.cs
@@ -0,0 +1,45 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Member Author

@liliankasem liliankasem Jan 24, 2026

Choose a reason for hiding this comment

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

Dupe from another PR, only change here is addition of:


    /// <summary>
    /// Gets or sets the serialized resource metadata.
    /// </summary>
    public string? Metadata { get; set; }

liliankasem
liliankasem commented Jan 24, 2026
View reviewed changes
src/Microsoft.Azure.Functions.Worker.Extensions.Mcp/Abstractions/ResourceInvocationContext.cs
/// <summary>
/// Represents the context for an MCP resource invocation.
/// </summary>
public sealed class ResourceInvocationContext
Copy link
Member Author

@liliankasem liliankasem Jan 24, 2026

Choose a reason for hiding this comment

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

Dupe from another PR, can ignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabiocav fabiocav fabiocav left review comments

Assignees

No one assigned

Labels

area: dotnet-worker Items impacting the .NET worker area: host-extension Items impacting the host extension

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@liliankasem @fabiocav