Attributes never returned are lost on creates and updates

[xjunior]

Since we started never returning the password attribute via the return = "never" [1] mechanism [].

At https://github.com/RIPAGlobal/scimitar/blob/v2.7.1/app/models/scimitar/resources/mixin.rb#L420, ci_scim_hash no longer contains the password, but after all operations are applied, an all attributes update is done on https://github.com/RIPAGlobal/scimitar/blob/v2.7.1/app/models/scimitar/resources/mixin.rb#L494 using from_scim, targeting all attributes in the resource, including those never returned, and emptying them.

The included test case ilustrates the scenario.

[1] 413280a

Fixes #105

[xjunior]

This seem similar to what is being discussed in #6

[pond]

Thanks for this submission. Unfortunately the PR contains changes that make it hard for me to understand what's needed and what's an accident. For example at the very top of the diff line 142 loses "self." for no apparent reason. The placement of "password" in parameter lis ets elsewhere has changed. Why; surely the hash position isn't significant?

I'll review this properly once extraneous diff entries have been removed - please do make pass through and revert any changes that are not strictly part of the change set for the fix being produced here, even if this might not match you personal preferences for code style.

If it's not a hassle, please could you annotate the diff with comments too, for example to explain why READ_WRITE_ATTRIBUTES changed to WRITE_ATTRIBUTES in tests?

[xjunior]

@pond nothing really is an accident, but the thick of it is reverting this commit. While this commit and this commit add specs that are broken without the said revert. This PR is mostly an exploratory work to fix the issues presented on the specs added by the above commits, and I'm not suggesting we revert the mentioned commit, but that it's where the bug was introduced. Sorry about the confusion.

[pond]

@xjunior I'm still a bit in the dark. #80 implements the password returning fix. All tests pass; v1 and main to this day pass every test, and always must else a PR cannot merge. I think you're trying to say that this PR here adds some tests to prove a bug and that's awesome, but just reverting #80 is definitely not a fix because it means the password attribute is being returned again.

[xjunior]

I think you're trying to say that this PR here adds some tests to prove a bug and that's awesome:

Yes, that's what I said:

While this commit and this commit add specs that are broken without the said revert.

This PR adds these two commits, which proves that there's a bug while patching a resource with an attribute marked as 'return=never', in this case, User and password.

but just reverting #80 is definitely not a fix because it means the password attribute is being returned again.

I agree. This PR proves both: 1) there's a bug (and now a spec asserting that); 2) the bug was caused by that change.

This PR does not aim at solving both issues, but to expose one.

[pond]

Yeah, I mean it needs fixing clearly (though it must be less urgent than I would expect, given the apparent severity, as this has gone unreported otherwise) but don't want to regress the non-returnable attributes PR in the process.

I'm trying to figure out how to fix it but the day has been constant interruptions and I'm really just doing nothing much but making mistakes now due to tiredness, annoyingly. Basically it comes down to as_json being the wrong place; it's to_scim, I think, that should be filtering. The trouble is, this only has schema access at the top level, then calls a private backend method for recursion through nested attributes and inside there, it has no visibility of the nested schema at each level. A custom schema that tried to not return something deeper than top-level wouldn't work.

Incidentally, forgive me for missing the point somewhat at first - but - random commits don't mean much out of context to anyone but the person that committed them. If a specific commit is mentioned, the "owning" PR ideally is mentioned too, so a reader knows the details. It took a while to dig out PR 80 as the root of all this as a result, and that's why I wasn't sure about the two tests you mentioned only by commit (at first glance).

[pond]

...and the start of that is in main...feature/fix-non-returnables, very much a WIP. The change to not return an attribute in the Name complex type is to prove the nesting failure (spec/models/scimitar/resources/mixin_spec.rb:163 passes but shouldn't).

Interestingly I think there was always a bug in spec/models/scimitar/resources/base_spec.rb at line 145, in the style of "how did that ever pass"; privateName is a returned: false attribute but line 153 expects it to be present in the PR 80 version of as_json. Surely, it should've expected that to be absent. Colour me confused.

[xjunior]

Thanks for your further analysis on this, @pond! I have temporary fixes for the password on both create and patch, but wanted to bring this up here. I agree, it comes down to as_json. While working on this, one idea I had was to pass a reason for the SCIM serialization, that way we could render the returned=never attributes when rendering the SCIM model for writing purposes – but I despised this solution.

Sorry about the initial confusion on the description of this issue, I'm glad you could end up understanding the problem.

[pond]

I had exactly the same idea initially - render because of a write op vs a read op, say - but I couldn't see a way to do it that didn't need controller changes and that'd break existing clients. Next, I thought about checking the action name but - euw, even worse. There shouldn't be any non-standard actions for SCIM but who knows, there might be in some cases. So that was a bad plan.

Anyway, the as-JSON / to-SCIM stuff gets used a lot. The gem's founding components have that strange split and some of it wasn't documented, so it's always been something of a guessing game. Part of the work I want to do here is to better document what you use and when. I'd rename as_json at this point except it's in use by client code so I can't; perhaps I'll add an alias instead; it's confusing, of course, since to_json is already a Thing and that's what you actually want most of the time.

TL;DR It made my head hurt. Much of what I got by building Scimitar from other implementations was very useful and constructed in ways I wouldn't have thought of, but some other areas are definitely not built how I'd have done it. Were it not for a rush to get a SCIM implementation into our product (which of course was then never used as the client changed their minds! LOL) I might've taken more care there.

[xjunior]

I had exactly the same idea initially - render because of a write op vs a read op, say - but I couldn't see a way to do it that didn't need controller changes and that'd break existing clients. Next, I thought about checking the action name but - euw, even worse. There shouldn't be any non-standard actions for SCIM but who knows, there might be in some cases. So that was a bad plan.

Anyway, the as-JSON / to-SCIM stuff gets used a lot. The gem's founding components have that strange split and some of it wasn't documented, so it's always been something of a guessing game. Part of the work I want to do here is to better document what you use and when. I'd rename as_json at this point except it's in use by client code so I can't; perhaps I'll add an alias instead; it's confusing, of course, since to_json is already a Thing and that's what you actually want most of the time.

TL;DR It made my head hurt. Much of what I got by building Scimitar from other implementations was very useful and constructed in ways I wouldn't have thought of, but some other areas are definitely not built how I'd have done it. Were it not for a rush to get a SCIM implementation into our product (which of course was then never used as the client changed their minds! LOL) I might've taken more care there.

I feel your pain, on both ways. I can think of ways to improve Scimitar too, starting by adding some abstractions on top of the mapping process.

One of the improvements I could be doing in the midterm, though, would be what I suggested in this issue: #107

[pond]

@xjunior I think this is looking decent:

main...feature/fix-non-returnables

EDIT - also generally concur that in terms of priorities, refactoring is all well and good but implementing as major feature as bulk updates would definitely seem a more valuable use of time! :-D

[xjunior]

@pond that PR is looking good, and going in a good direction too!

[xjunior]

Please close this PR when that is open. Also, looks like it introduces a recursive attribute path, which can be useful here.

[xjunior]

Closed in favor of #109