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

fix(csharp/FieldMask): Support map fields in c# FieldMask #19673

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ahmednfwela
Copy link

@ahmednfwela ahmednfwela commented Dec 15, 2024

fixes #11989

This PR adds support to referencing keys in a map field via field masks and introduces tests that used to fail before.

related issues:

AIP: https://google.aip.dev/161#map-fields

@ahmednfwela ahmednfwela changed the title tests(FieldMask): provide failing test cases for map fields fix(FieldMask): Fix missing map field handling in c# FieldMasks Dec 15, 2024
@ahmednfwela ahmednfwela changed the title fix(FieldMask): Fix missing map field handling in c# FieldMasks fix(c#/FieldMask): Fix missing map field handling in c# FieldMasks Dec 16, 2024
@ahmednfwela ahmednfwela changed the title fix(c#/FieldMask): Fix missing map field handling in c# FieldMasks fix(csharp/FieldMask): Fix missing map field handling in c# FieldMasks Dec 16, 2024
@ahmednfwela ahmednfwela marked this pull request as ready for review December 16, 2024 15:44
@ahmednfwela ahmednfwela requested a review from a team as a code owner December 16, 2024 15:44
@ahmednfwela ahmednfwela requested review from jskeet and removed request for a team December 16, 2024 15:44
@ahmednfwela
Copy link
Author

ahmednfwela commented Dec 16, 2024

Note for reviewers:

This PR fixes two related issues

  1. Merging FieldMasks that target a map field (referenced in Field Mask merge with a map fails #11989)
  2. Referencing a specific key for a map in a FieldMask (referenced in AIP 161 and FieldMask JSON serialization improvement proposal #12524)

The tests might seem repetitive, but they cover all possible branches of the introduced code, that's why I split them into a separate test class.

Future work:

  1. Support parsing back ticks: currently we do a simple split by . to determine paths, but in the future we can add support for backticks for invalid paths, e.g.:
// Valid field masks: reviews, reviews.smith, reviews.`John Smith`, reviews.`Dr. John Smith`
map<string, string> reviews = 2;

Also cc @jskeet since he worked on the original implementation

@ahmednfwela ahmednfwela changed the title fix(csharp/FieldMask): Fix missing map field handling in c# FieldMasks fix(csharp/FieldMask): Support map fields in c# FieldMask Dec 16, 2024
@jskeet
Copy link
Contributor

jskeet commented Dec 16, 2024

I'm not going to have time to review this PR this year, I'm afraid.

@ahmednfwela
Copy link
Author

I'm not going to have time to review this PR this year, I'm afraid.

That's ok, but would it be fine if you approved running the tests?

@jskeet
Copy link
Contributor

jskeet commented Dec 16, 2024

I'm not going to have time to review this PR this year, I'm afraid.

That's ok, but would it be fine if you approved running the tests?

Yup, will do that now.

@jskeet jskeet added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 16, 2024
@zhangskz zhangskz added the c# label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field Mask merge with a map fails
3 participants