Skip to content

Add AddtionalProperties for DataConnector #38644

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
wants to merge 2 commits into
base: release-app-Microsoft.App-stable/2026-01-01
Choose a base branch
from
Open

Add AddtionalProperties for DataConnector #38644

wants to merge 2 commits into from

Conversation

@LaylaLiu-gmail
Copy link
Contributor

@LaylaLiu-gmail LaylaLiu-gmail commented Nov 6, 2025

Choose a PR Template

Switch to "Preview" on this description then select one of the choices below.

Click here to open a PR for a Data Plane API.

Click here to open a PR for a Control Plane (ARM) API.

Click here to open a PR for only SDK configuration.

@github-actions
Copy link

github-actions bot commented Nov 6, 2025 •
edited
Loading

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ This PR is in purview of the ARM review (label: ARMReview). This PR must get ARMSignedOff label from an ARM reviewer.
    This PR is awaiting ARM reviewer feedback (label: WaitForARMFeedback).
    To learn when this PR will get reviewed, see ARM review queue at aka.ms/azsdk/pr-arm-review
    For details of the ARM review, see aka.ms/azsdk/pr-arm-review
  • ❌ The required check named Swagger ModelValidation has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide


Comment generated by summarize-checks workflow run.

@github-actions github-actions bot added brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. ARMReview resource-manager WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 6, 2025
@LaylaLiu-gmail LaylaLiu-gmail force-pushed the user/layliu/AdditionalProperties branch 2 times, most recently from 7d5ec99 to e868819 Compare November 6, 2025 09:57
@LaylaLiu-gmail LaylaLiu-gmail force-pushed the user/layliu/AdditionalProperties branch from e868819 to 826cd62 Compare November 6, 2025 12:55
gary-x-li
gary-x-li reviewed Nov 6, 2025
View reviewed changes
...on/app/resource-manager/Microsoft.App/ContainerApps/stable/2026-01-01/CommonDefinitions.json
"modelAsString": true
}
},
"extendedProperties": {
Copy link
Contributor

@gary-x-li gary-x-li Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a collection of key value pairs without concrete schema. Also, it lacks enough description and examples. What are these key value pairs? Can you define proper schema for them?

Copy link
Contributor Author

@LaylaLiu-gmail LaylaLiu-gmail Nov 7, 2025

Choose a reason for hiding this comment

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

@gary-x-li, actually we need a property who is a Dictionary<string, object>. but addtionalProperties is not recommended by swagger. Is there any other way to define such a dictionary in swagger?

Copy link
Member

@najian najian Nov 7, 2025

Choose a reason for hiding this comment

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

More explanation:

  1. extendedproperties in Data connector represent various additional option highly dependent on the specific data connector being used. It's common industrial practice to make the data connector protocol extensible, like JDBC, ODBC...
  2. all the elements to be treated as an atomic unit, non-ordered.
  3. the property set represents a simplistic array with a very small number of elements that are not expected to grow drastically over time and would never have paging requirements.

Copy link
Contributor

@gary-x-li gary-x-li Nov 7, 2025

Choose a reason for hiding this comment

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

Can you please do/answer the followings?

  1. Add or modify example files to show what extended properties look like for at least two different data connectors. That will make it clearer to the reviewers and the customers.
  2. How would customers know what the valid set of extended properties for each data connector are? Are they widely known as industrial practice? Consider expanding the description to give customers better idea.
  3. We do allow additionalProperties if the properties are simply passing through your service and you don't control nor validate them. (e.g. tags for Azure resources or labels for Kubernetes). If that's your case, please do use additionalProperties and suppress the validation error. We will approve the suppression if it's justified.

Copy link
Contributor Author

@LaylaLiu-gmail LaylaLiu-gmail Nov 10, 2025

Choose a reason for hiding this comment

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

Have updated the example files and this is a pass-through property. our RP doesn't do any validation on this property. So, I just changed it to dictionary for better understanding.

Copy link
Contributor

@sandipsh sandipsh Nov 10, 2025

Choose a reason for hiding this comment

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

3rd point is not yet addressed.

Copy link
Contributor

@gary-x-li gary-x-li Nov 10, 2025

Choose a reason for hiding this comment

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

In addition, I also don't see 1st point (adding example files) and 2nd point (change description to indicate where to get the valid set of properties).

Copy link
Contributor Author

@LaylaLiu-gmail LaylaLiu-gmail Nov 11, 2025

Choose a reason for hiding this comment

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

sorry, I pushed the latest change to another branch. Now it has the latest change. but the validation shows "the type is invalid. expected object, but found string, array". Actually, we cannot specify the value type, the value could be string or array. That's why we used object. @gary-x-li , do you know how to solve such issue?

Copy link
Contributor

@gary-x-li gary-x-li Nov 11, 2025

Choose a reason for hiding this comment

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

@LaylaLiu-gmail I'm not sure. You can post your question in this channel; or search similar examples in the repo.

@gary-x-li gary-x-li added ARMChangesRequested and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 6, 2025
@najian najian added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed ARMChangesRequested labels Nov 7, 2025
@gary-x-li gary-x-li added ARMChangesRequested and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 7, 2025
@LaylaLiu-gmail LaylaLiu-gmail added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed ARMChangesRequested labels Nov 10, 2025
@sandipsh sandipsh added ARMChangesRequested and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 10, 2025
@github-actions
Copy link

github-actions bot commented Nov 11, 2025 •
edited
Loading

API Change Check

APIView identified API level changes in this PR and created the following API reviews

Language API Review for Package
Go sdk/resourcemanager/appcontainers/armappcontainers
Java com.azure.resourcemanager:azure-resourcemanager-appcontainers
JavaScript @azure/arm-appcontainers

@LaylaLiu-gmail LaylaLiu-gmail added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed ARMChangesRequested labels Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gary-x-li gary-x-li gary-x-li left review comments

@najian najian najian left review comments

@sandipsh sandipsh sandipsh left review comments

@Juliehzl Juliehzl Awaiting requested review from Juliehzl Juliehzl is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

ARMReview brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. resource-manager WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@LaylaLiu-gmail @gary-x-li @najian @sandipsh