Skip to content

feat: support dot notation for add and replace ops#46

Merged
ivixvi merged 1 commit intoivixvi:mainfrom
Valodim:dot-notation
Jul 3, 2025
Merged

feat: support dot notation for add and replace ops#46
ivixvi merged 1 commit intoivixvi:mainfrom
Valodim:dot-notation

Conversation

@Valodim
Copy link
Copy Markdown
Contributor

@Valodim Valodim commented Jul 3, 2025

Hey there! Thanks for taking a stab at scim-patch in golang 👍

I noticed that there is no support for the "dot notation", which is prominently used by MS Entra. Here is an example patch operation from their validator:

    {
      "op": "replace",
      "value": {
        "externalId": "39be3336-3e88-4c41-80b7-c654ab698a5e",
        "active": true,
        "name.familyName": "Lincoln",
        "name.givenName": "Greg"
      }

The name.familyName here should be applied to the familyName subattribute of name. This dot notation has no examples inthe spec unfortunately, but is mentioned in Section 3.10. There are also some examples of how dot notation is used in the entra scim compatibility docs, see the "Requests to replace multiple attributes" example with the new feature flag (now enabled by default, I believe).

This PR adds support for the dot notation and some tests for it. I'm not sure the way it is implemented fully matches your architecture, but if it doesn't perhaps this can serve as a starting point.

I also validated that it fixes the relevant step of the entra scim validator.

image

@ivixvi ivixvi self-requested a review July 3, 2025 11:46
Copy link
Copy Markdown
Owner

@ivixvi ivixvi left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for this great contribution 👍 — really appreciate you taking the time to work on scim-patch and especially for including the helpful references!

You're right that the SCIM RFC doesn't explicitly define dot notation usage in the value field, but after reviewing the Entra ID documentation and validator behavior, I agree that supporting this is a meaningful enhancement. I'd like to move forward with incorporating it.

I’ve added two points from an architectural perspective — please take a look and let me know your thoughts.

Again, thanks for the thoughtful PR and tests — looking forward to collaborating more!

Comment thread dot.go Outdated
Comment thread adder.go Outdated
Comment on lines +14 to +15
scopedMap, scopedAttr = applyDotNotation(scopedMap, scopedAttr)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In the current design, we generally delegate value resolution and movement to scope.go or scimpatch.go, so I think it would make sense to handle dot notation there as well.

My current thought is that it might be most natural to invoke applyDotNotation inside pathUnspecifiedOperate in scimpatch.go, to resolve and re-map the paths accordingly.

Copy link
Copy Markdown
Contributor Author

@Valodim Valodim Jul 3, 2025

Choose a reason for hiding this comment

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

I had also considered that, but it seemed like I would just move it to all call sites of Direct then.

Do you think this only applies to the pathUnspecifiedOperate flow? It does for entra (I believe), but in theory it could apply in cases that have a path as well 🤔

If we want to support this for ops with no path only, moving it up to scimpatch.go is the simple way to go.

Copy link
Copy Markdown
Owner

@ivixvi ivixvi Jul 3, 2025

Choose a reason for hiding this comment

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

it seemed like I would just move it to all call sites of Direct then.

in theory it could apply in cases that have a path as well 🤔

Yeah, exactly... 😢
I totally get that calling resolveDotNotation before every use of Direct doesn’t feel great either.

That said, I also feel a bit uneasy about baking that logic directly into Operators just for the sake of simplification.

As an alternative, how about introducing a wrapper layer around Direct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gave it a shot, it's actually not so bad. We add it to all call sites, but now it doesn't have to be in every operator. So overall, it's fine imo

PTAL

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the update!

I think so too.
If any issues come up in future changes, we can revisit it then.

Copy link
Copy Markdown
Owner

@ivixvi ivixvi left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks again for taking the time to work on this!

@ivixvi ivixvi merged commit 1f33998 into ivixvi:main Jul 3, 2025
1 check passed
@Valodim Valodim deleted the dot-notation branch July 3, 2025 14:32
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.

2 participants