Allow requesting specific attributes (derived from #102)

RIPAGlobal / scimitar

A SCIM v2 API endpoint implementation

MIT License

57 stars 37 forks source link

Allow requesting specific attributes (derived from #102) #127

Closed pond closed 3 months ago

pond commented 3 months ago

This is derived from #102 with:

Some minor commentary fixes, including some drive-bys from when I was reviewing existing docs as part of understanding #102 and wider context
A change in #102's blanket removal of nil values. There's a small chance that some existing Scimitar clients might be relying on "key is present, but value is nil" in returned data, so nil items are now only stripped out if an include-attributes list is included; otherwise, nil handling behaviour is unchanged.

Many thanks to @xjunior for this work.

pond commented 3 months ago

@sierra-alpha

I don't see anything asserting this in the new tests

Yes, "missing" diffs can be tricky things! If you look at https://github.com/RIPAGlobal/scimitar/pull/102/files#diff-f82acfe1ea74bff57d9a8330817ac4f7ed9b8a027d75a1ff36c9c0dd47cf5408R330 you'll see that in #102, tests which did check for nil entries were updated to no longer do so. Those changes were reverted, since the nil entries are still expected. As a result, there's no alteration to those tests anymore, so nothing to show in the diff - which I suppose in a way, you'd expect, given that the behaviour is supposed to be unchanged unless an attribute inclusion list is specified.

xjunior commented 3 months ago

Hey, @pond. It's great to see this moving, thank you!

For the removed attributes, you're right. Nevertheless, JSON clients could assume a difference between a present and a null value.

I'll look into removing these attributes from the output JSON when attributes are requested, so we get a cleaner JSON when needed in a future PR, this was the main goal of this blanket removal of nil values, but I agree it wasn't the best solution.

pond commented 3 months ago

@xjunior

I'll look into removing these attributes from the output JSON when attributes are requested, so we get a cleaner JSON when needed in a future PR, this was the main goal of this blanket removal of nil values, but I agree it wasn't the best solution.

That's what I've implemented. The nil removal code is still there, it just only runs if an attribute map is included.

https://github.com/RIPAGlobal/scimitar/pull/127/files#diff-e137e2371ae195d13d92508cf5e64cedd5c379c72fd9387a2b9c814d6199c2ebR604

That's really (aside from some minor formatting changes) the only difference between this and your PR (which is the starting basis in this feature branch of the code here).

xjunior commented 3 months ago

Perfect, thanks @pond ! Looking forward to incorporate this on our project!