Fix non-returnables #109
Fix non-returnables #109
Conversation
| # | ||
| # ...so all fields, even those marked "returned: false", are included. | ||
| # Use Scimitar::Resources::Mixin::to_scim to obtain a SCIM object with | ||
| # returnable fields omitted, rendering *that* as JSON via #to_json. |
There was a problem hiding this comment.
This is stil somewhat clear as mud, but at least it gives a fairly strong indication that "you probably don't want to call this for returned JSON data". Between this and the clear examples of controller code in README.md, hopefully it's enough for now.
| non_returnable_attributes << 'errors' | ||
|
|
||
| original_hash = super(options).except(*non_returnable_attributes) | ||
| original_hash = super(options).except('errors') |
There was a problem hiding this comment.
Reverts the problematic part of #80.
|
|
||
| resource.meta = Meta.new(meta_attrs_hash) | ||
| return resource | ||
| return self.to_scim_for_returning(location: location) |
There was a problem hiding this comment.
This gives away that we can't actually fully avoid internally the concept of to-SCIM in "render normally" vs "render everything" mode; more info in later diff comments.
There was a problem hiding this comment.
I guess it's fine for now and more clarity helps in future refactoring.
| @@ -417,7 +408,7 @@ def from_scim!(scim_hash:) | |||
| # | |||
| def from_scim_patch!(patch_hash:) | |||
| frozen_ci_patch_hash = patch_hash.with_indifferent_case_insensitive_access().freeze() | |||
| ci_scim_hash = self.to_scim(location: '(unused)').as_json().with_indifferent_case_insensitive_access() | |||
| ci_scim_hash = self.to_scim_for_patch_op().as_json().with_indifferent_case_insensitive_access() | |||
There was a problem hiding this comment.
Idiomatic SCIM resource controller code for an #update operation has this at the core:
record = self.find_record(record_id)
record.from_scim_patch!(patch_hash: patch_hash)
self.save!(record, &block)
# ...and render 'record' in SCIM form...That's how we get to the method being changed here. At the end of this method - you'd need to expand the diff downwards to around new diff line 485 or so - the final steps are:
self.from_scim!(scim_hash: ci_scim_hash)
return selfThe idea is that we build ci_scim_hash from the existing data as per the line above, then modify it via whatever update operations are requested, then repopulate the resource from that updated hash. This is related to long-standing issue #6. In lieu of the workload required to produce a true fix, and given that the mixin's tests do include tests to say "if a value is omitted in the Hash then it is set to nil", we can't allow ci_scim_hash to have missing keys due to non-rendered "returned never" attributes. Instead, it must contain them all, so that the current relatively crude (but still, thanks to SCIM's design, very complex) implementation can work as expected.
I note in passing that in the special case of a password, when something like Devise is in use and being used properly (!) then, once a password is written it is encrypted and cannot be read. Attempts to read the password or confirmation return nil; attempts to write nil or empty string have no side effects and are ignored. This is the desired outcome for that attribute. It does open up the question of non-password "returned never" attributes where the underling model might not allow a read operation, though, causing set-to-nil behaviour in subsequent SCIM patch updates.
TL;DR, what a mess. But this gets us by for now, and resolves issues highlighted by the updated tests.
|
@xjunior This is ready to be looked at now. I've annotated the diff with a few comments. It's interesting to see that a reliable, deep fix would only be possible with issue #6 addressed, but as-is, unless people are using very weird schema, it should do just fine for common user and group management workloads. |
|
Converted back to draft after a giant rabbit hole opened, in which a hundred yaks in need of shaving. |
|
@xjunior this is - I think! - ready for review now. In addition, but only if you have time, please could you try using this in your gem 'scimitar', git: 'https://github.com/RIPAGlobal/scimitar', branch: 'feature/fix-non-returnables' |
|
@pond I just ran my test suite with this branch and my specs are passing without the hacks I had. |
lib/scimitar/support/hash_with_indifferent_case_insensitive_access.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Shaun Alexander <37967220+sierra-alpha@users.noreply.github.com>
…cess.rb Co-authored-by: Shaun Alexander <37967220+sierra-alpha@users.noreply.github.com>
Amends a bug introduced in #80 via a variation of suggested fix #103, which reports the quite serious bug that on-create/update attributes like
passwordare being ignored. See #103 for conversation history - TL;DR, looks likeas_jsonwas the wrong place andto_scimis the right one for the masking out of non-returnable attributes.Full credit to @xjunior for #103 and getting the ball rolling on this.