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

Update patch.go #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

zakirhussain
Copy link

For patch request, It is actually modifying reference which is received from the DB

For patch request, It is actually modifying reference which will be received from the DB
@zakirhussain
Copy link
Author

I have tested and verified for in memory DB

atheriel added a commit to posit-dev/go-scim that referenced this pull request Apr 21, 2022
At present all PATCH requests for resources that have a `meta.version`
attribute will fail with ErrConflict because the PatchService.Do()
method incorrectly mutates the current resource instead of its
replacement.

This error was not caught by existing unit tests because they do not set
a `meta.version` attribute and db.Memory().Replace() has the following
check:

    version := ref.MetaVersionOrEmpty()
    if len(version) > 0 && m.db[id].MetaVersionOrEmpty() != version {
    	return spec.ErrConflict
    }

This has the effect of permitting replacements when there is no version
present -- which is the case for the existing unit tests **but not real
APIs**. Simply adding a `meta.version` attribute to unit tests (as in
this commit) yields failures like as the following:

    === RUN   TestPatchService/TestDo/patch_to_make_a_difference
        patch_test.go:99:
            	Error Trace:	patch_test.go:99
            	            				patch_test.go:366
            	Error:      	Expected nil, but got: &spec.Error{Status:412, Type:"conflict"}
            	Test:       	TestPatchService/TestDo/patch_to_make_a_difference

An identical fix was originally proposed by @zakirhussain in imulab#80. This
commit expands on that work to include unit test changes to catch a
regression and (hopefully) a better explanation on the underlying cause
of the issue.

Co-authored-by: Zakir Hussain <[email protected]>
Signed-off-by: Aaron Jacobs <[email protected]>
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.

1 participant