elimity-com / scim

Golang Implementation of the SCIM v2 Specification
MIT License
177 stars 55 forks source link

Question: is implementing a patch application function reasonable? #171

Open icamys opened 8 months ago

icamys commented 8 months ago

I was looking at how to implement data patching, and it appears that a working algorithm looks as follows:

  1. Get the patch operations in the HTTP handler (the package already does this);
  2. Get data you want to patch from a data store and transform it to a map[string]interface{};
  3. Apply the patch operations to the data;
  4. Validate the patched data using Schema.Validate() function. If not valid, return a patch error;
  5. Transform the patched data to resource and return the resource.

If this is how the package is designed to work, it seems reasonable to implement a generic function that does data patching. Something like this:

// Apply applies the patch operation to the provided data.
func (o *PatchOperation) Apply (data map[string]interface{})

Then, the handler will have a shape like this:

func (h *Handler) Patch(r *http.Request, id string, operations []scim.PatchOperation) (scim.Resource, error) {
    entity := h.dataStore.Get(id)

        data := userEntity.ToMap()

        for _, op := range operations {
                op.Apply(data)
        }

        if err := h.schema.Validate(data); err != nil {
                // return an appropriate error
        }

       entity := newEntityFromData(data)
       h.dataStore.Save(entity)

       resource := newResourceFromEntity(entity)

    return resource, nil
}
q-uint commented 7 months ago

Hi @icamys,

The reason we have not immediately added it, is because we use custom structs to represent data, and do not use maps for resource types. So implementing PATCH for it made more sense at the time than converting it to a map and back to do this.

It's important to note that every user's data store varies, and while this feature may not be universally applicable, it could certainly benefit certain users.

icamys commented 7 months ago

@q-uint Thank you for the response. It makes sense.

Although the data store varies, I suppose, in most cases, the data model returned from the data store can be transformed into a map. At least, if theoretically there were a function that automatically applies a patch to a map, creating functions that convert data models to maps and back would be beneficial for the users of this package.

What's the current recommended approach to applying patches? Is it something like this?:

var entity *User

// for each patch operation
switch op {
case "add":
    addAttribute(entity, path, value)
case "remove":
    removeAttribute(entity, path, value)
case "replace":
    replaceAttribute(entity, path, value)
}

// where addAttribute, removeAttribute, and replaceAttribute know 
// how to apply a certain patch to a *User entity type