Make conditional vs unconditional updates clearer (#48359)

* Make conditional vs unconditional updates clearer

* address edoardo's feedback

lib/services/local/access_monitoring_rules.go

@@ -85,7 +85,7 @@ func (s *AccessMonitoringRulesService) CreateAccessMonitoringRule(ctx context.Co
 
 // UpdateAccessMonitoringRule updates an existing AccessMonitoringRule resource.
 func (s *AccessMonitoringRulesService) UpdateAccessMonitoringRule(ctx context.Context, amr *accessmonitoringrulesv1.AccessMonitoringRule) (*accessmonitoringrulesv1.AccessMonitoringRule, error) {
-	updated, err := s.svc.UpdateResource(ctx, amr)
+	updated, err := s.svc.UnconditionalUpdateResource(ctx, amr)
 	return updated, trace.Wrap(err)
 }
 

lib/services/local/databaseobject.go

@@ -45,7 +45,7 @@ func (s *DatabaseObjectService) UpsertDatabaseObject(ctx context.Context, object
 }
 
 func (s *DatabaseObjectService) UpdateDatabaseObject(ctx context.Context, object *dbobjectv1.DatabaseObject) (*dbobjectv1.DatabaseObject, error) {
-	out, err := s.service.UpdateResource(ctx, object)
+	out, err := s.service.UnconditionalUpdateResource(ctx, object)
 	return out, trace.Wrap(err)
 }
 

lib/services/local/databaseobjectimportrule.go

@@ -43,7 +43,7 @@ func (s *databaseObjectImportRuleService) UpsertDatabaseObjectImportRule(ctx con
 }
 
 func (s *databaseObjectImportRuleService) UpdateDatabaseObjectImportRule(ctx context.Context, rule *databaseobjectimportrulev1.DatabaseObjectImportRule) (*databaseobjectimportrulev1.DatabaseObjectImportRule, error) {
-	out, err := s.service.UpdateResource(ctx, rule)
+	out, err := s.service.UnconditionalUpdateResource(ctx, rule)
 	return out, trace.Wrap(err)
 }
 

lib/services/local/generic/generic_wrapper.go

@@ -125,14 +125,20 @@ func (s ServiceWrapper[T]) UpsertResource(ctx context.Context, resource T) (T, e
 	return adapter.resource, trace.Wrap(err)
 }
 
-// UpdateResource updates an existing resource.
-func (s ServiceWrapper[T]) UpdateResource(ctx context.Context, resource T) (T, error) {
+// UnconditionalUpdateResource updates an existing resource without checking the provided resource revision.
+// Because UnconditionalUpdateResource can blindly overwrite an existing item, ConditionalUpdateResource should
+// be preferred.
+// See https://github.com/gravitational/teleport/blob/master/rfd/0153-resource-guidelines.md#update-1 for more details
+// about the Update operation.
+func (s ServiceWrapper[T]) UnconditionalUpdateResource(ctx context.Context, resource T) (T, error) {
 	adapter, err := s.service.UpdateResource(ctx, newResourceMetadataAdapter(resource))
 	return adapter.resource, trace.Wrap(err)
 }
 
 // ConditionalUpdateResource updates an existing resource if the provided
 // resource and the existing resource have matching revisions.
+// See https://github.com/gravitational/teleport/blob/master/rfd/0126-backend-migrations.md#optimistic-locking for more
+// details about the conditional update.
 func (s ServiceWrapper[T]) ConditionalUpdateResource(ctx context.Context, resource T) (T, error) {
 	adapter, err := s.service.ConditionalUpdateResource(ctx, newResourceMetadataAdapter(resource))
 	return adapter.resource, trace.Wrap(err)

lib/services/local/generic/generic_wrapper_test.go

@@ -180,7 +180,7 @@ func TestGenericWrapperCRUD(t *testing.T) {
 
 	// Update a resource.
 	r1.Metadata.Labels = map[string]string{"newlabel": "newvalue"}
-	r1, err = service.UpdateResource(ctx, r1)
+	r1, err = service.UnconditionalUpdateResource(ctx, r1)
 	require.NoError(t, err)
 	r, err = service.GetResource(ctx, r1.GetMetadata().GetName())
 	require.NoError(t, err)
@@ -198,7 +198,7 @@ func TestGenericWrapperCRUD(t *testing.T) {
 
 	// Update a resource that doesn't exist.
 	doesNotExist := newTestResource153("doesnotexist")
-	_, err = service.UpdateResource(ctx, doesNotExist)
+	_, err = service.UnconditionalUpdateResource(ctx, doesNotExist)
 	require.True(t, trace.IsNotFound(err))
 
 	// Delete a resource.