Fix PATCH conflicts when meta.version is present.

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 <pro.zakir24@gmail.com>
Signed-off-by: Aaron Jacobs <aaron.jacobs@rstudio.com>

Author: atheriel

pkg/v2/service/patch.go

@@ -83,13 +83,13 @@ func (s *patchService) Do(ctx context.Context, req *PatchRequest) (resp *PatchRe
 		return
 	}
 
-	resource, err := s.database.Get(ctx, req.ResourceID, nil)
+	ref, err := s.database.Get(ctx, req.ResourceID, nil)
 	if err != nil {
 		return
 	}
 
 	if s.config.ETag.Supported && req.MatchCriteria != nil {
-		if !req.MatchCriteria(resource) {
+		if !req.MatchCriteria(ref) {
 			err = fmt.Errorf("%w: resource does not meet pre condition", spec.ErrConflict)
 			return
 		}
@@ -98,7 +98,7 @@ func (s *patchService) Do(ctx context.Context, req *PatchRequest) (resp *PatchRe
 	// To save another database round trip, we use Clone to retain independent copy of the fetched resource.
 	// However, because the cloned instance share subscribers, it is better to work on the original instance.
 	// Hence, we assign reference to the clone, which will not be modified.
-	ref := resource.Clone()
+	resource := ref.Clone()
 
 	for _, f := range s.preFilters {
 		if err = f.FilterRef(ctx, resource, ref); err != nil {

pkg/v2/service/patch_test.go

@@ -43,6 +43,13 @@ func (s *PatchServiceTestSuite) TestDo() {
 				err := database.Insert(context.TODO(), s.resourceOf(t, map[string]interface{}{
 					"schemas":  []interface{}{"urn:ietf:params:scim:schemas:core:2.0:User"},
 					"id":       "foo",
+					"meta": map[string]interface{}{
+						"resourceType": "User",
+						"created":      "2019-11-20T13:09:00",
+						"lastModified": "2019-11-20T13:09:00",
+						"location":     "https://identity.imulab.io/Users/foo",
+						"version":      "W/\"1\"",
+					},
 					"userName": "foo",
 					"timezone": "Asia/Shanghai",
 					"emails": []interface{}{
@@ -90,6 +97,7 @@ func (s *PatchServiceTestSuite) TestDo() {
 			},
 			expect: func(t *testing.T, resp *PatchResponse, err error) {
 				assert.Nil(t, err)
+				require.NotNil(t, resp)
 				assert.True(t, resp.Patched)
 				assert.NotEmpty(t, resp.Resource.MetaVersionOrEmpty())
 				assert.NotEqual(t, resp.Ref.MetaVersionOrEmpty(), resp.Resource.MetaVersionOrEmpty())
@@ -105,6 +113,13 @@ func (s *PatchServiceTestSuite) TestDo() {
 				err := database.Insert(context.TODO(), s.resourceOf(t, map[string]interface{}{
 					"schemas":  []interface{}{"urn:ietf:params:scim:schemas:core:2.0:User"},
 					"id":       "foo",
+					"meta": map[string]interface{}{
+						"resourceType": "User",
+						"created":      "2019-11-20T13:09:00",
+						"lastModified": "2019-11-20T13:09:00",
+						"location":     "https://identity.imulab.io/Users/foo",
+						"version":      "W/\"1\"",
+					},
 					"userName": "foo",
 					"timezone": "Asia/Shanghai",
 					"emails": []interface{}{
@@ -143,6 +158,7 @@ func (s *PatchServiceTestSuite) TestDo() {
 			},
 			expect: func(t *testing.T, resp *PatchResponse, err error) {
 				assert.Nil(t, err)
+				require.NotNil(t, resp)
 				assert.False(t, resp.Patched)
 			},
 		},
@@ -153,6 +169,13 @@ func (s *PatchServiceTestSuite) TestDo() {
 				err := database.Insert(context.TODO(), s.resourceOf(t, map[string]interface{}{
 					"schemas":  []interface{}{"urn:ietf:params:scim:schemas:core:2.0:User"},
 					"id":       "foo",
+					"meta": map[string]interface{}{
+						"resourceType": "User",
+						"created":      "2019-11-20T13:09:00",
+						"lastModified": "2019-11-20T13:09:00",
+						"location":     "https://identity.imulab.io/Users/foo",
+						"version":      "W/\"1\"",
+					},
 					"userName": "foo",
 					"timezone": "Asia/Shanghai",
 					"emails": []interface{}{
@@ -200,6 +223,7 @@ func (s *PatchServiceTestSuite) TestDo() {
 			},
 			expect: func(t *testing.T, resp *PatchResponse, err error) {
 				assert.Nil(t, err)
+				require.NotNil(t, resp)
 				assert.True(t, resp.Patched)
 				assert.NotEmpty(t, resp.Resource.MetaVersionOrEmpty())
 				assert.NotEqual(t, resp.Ref.MetaVersionOrEmpty(), resp.Resource.MetaVersionOrEmpty())
@@ -215,6 +239,13 @@ func (s *PatchServiceTestSuite) TestDo() {
 				err := database.Insert(context.TODO(), s.resourceOf(t, map[string]interface{}{
 					"schemas":  []interface{}{"urn:ietf:params:scim:schemas:core:2.0:User"},
 					"id":       "foo",
+					"meta": map[string]interface{}{
+						"resourceType": "User",
+						"created":      "2019-11-20T13:09:00",
+						"lastModified": "2019-11-20T13:09:00",
+						"location":     "https://identity.imulab.io/Users/foo",
+						"version":      "W/\"1\"",
+					},
 					"userName": "foo",
 					"timezone": "Asia/Shanghai",
 					"emails": []interface{}{
@@ -255,6 +286,7 @@ func (s *PatchServiceTestSuite) TestDo() {
 			},
 			expect: func(t *testing.T, resp *PatchResponse, err error) {
 				assert.Nil(t, err)
+				require.NotNil(t, resp)
 				assert.True(t, resp.Patched)
 				assert.Equal(t, "bar", resp.Resource.Navigator().Dot("userName").Current().Raw())
 				assert.Nil(t, bcrypt.CompareHashAndPassword(
@@ -273,6 +305,13 @@ func (s *PatchServiceTestSuite) TestDo() {
 						"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User",
 					},
 					"id":       "foo",
+					"meta": map[string]interface{}{
+						"resourceType": "User",
+						"created":      "2019-11-20T13:09:00",
+						"lastModified": "2019-11-20T13:09:00",
+						"location":     "https://identity.imulab.io/Users/foo",
+						"version":      "W/\"1\"",
+					},
 					"userName": "foo",
 					"emails": []interface{}{
 						map[string]interface{}{
@@ -313,6 +352,7 @@ func (s *PatchServiceTestSuite) TestDo() {
 			},
 			expect: func(t *testing.T, resp *PatchResponse, err error) {
 				assert.Nil(t, err)
+				require.NotNil(t, resp)
 				assert.True(t, resp.Patched)
 				assert.Equal(t, "6546579", resp.Resource.Navigator().Dot("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User").Dot("employeeNumber").Current().Raw())
 			},