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

Issue with PATCH #369

Open
Seemone opened this issue Jan 22, 2025 · 9 comments
Open

Issue with PATCH #369

Seemone opened this issue Jan 22, 2025 · 9 comments
Labels
question Question about Fuego. Not a bug nor a feature clearly identified

Comments

@Seemone
Copy link

Seemone commented Jan 22, 2025

PATCH, as opposed to PUT, should accept incomplete subsets of an entity (see the example here
A quick test shows that (of course) Fuego passes a full struct with fields that are not present in the PATCH body set to the type default values.
Examples such as petstore suggest to treat a default value as "unset" and skip setting the field in the main entity. This makes impossible to distinguish a field which is absent in the request (and thus should not be modified) from a field that is set to the default value in the request (and thus should be set to the default value).
A possible example would be add a method to ContextWithBody that returns a map with only the set values and/or a method to update a given struct with the values in the map.

@EwenQuim
Copy link
Member

Yeah you're right. In a code written in my company for a client, I use different types to represent the same entity.

type Book struct {
    Title string
    CopiesSold int
}

type BookPatch struct {
    Title *string // this way, if not provided at deserialization, it will be nil
    CopiesSold *int
}

// and even a different one for creation
type BookCreate struct {
    Title string
}

We have several Go types to represent the entity stored in DB. And in the service, I use squirrel to build a SQL request that matches only non-nil fields.

For another client, we used SQLc, so we also had different types for different DB requests.

IDK if it's REST-y but it just represents reality, and that's what we want.


In the long run, we could do an /extra package that use GET and POST handlers using a same type to craft a PATCH accordingly. What do you think about it?

@Seemone
Copy link
Author

Seemone commented Jan 23, 2025

The issue with that approach is that you need a structure for every different combination of attributes which means you need different endpoints as well, as each endpoint has only one input structure. This breaks CRUD principle and can get ugly very fast if your entities have lots of fields.
Adding a method to ContextWithBody that updates a structure field if there is a corresponding field in the request imho elegantly solves the problem of "PATCH as generic updater".

@EwenQuim EwenQuim added the question Question about Fuego. Not a bug nor a feature clearly identified label Jan 23, 2025
@EwenQuim
Copy link
Member

you need a structure for every different combination of attributes

No we don't. We just have one PATCH /books with a type BookPatch witch have optional fields, as mentioned above

@EwenQuim
Copy link
Member

EwenQuim commented Jan 23, 2025

Adding a method to ContextWithBody that updates a structure field if there is a corresponding field in the request imho elegantly solves the problem of "PATCH as generic updater".

I'll try that, this can be really nice! How would you see the API? Something like that?

func (rs PetsResources) patchPets(c fuego.ContextWithBody[models.PetsPatch]) (*models.Pets, error) {
	id := c.PathParam("id")

        oldPet, _ := rs.PetsService.Get(id)

	patchedPet, err := c.PatchBody(oldPet) // Instead of Body and relying on an object to patch
	if err != nil {
		return nil, err
	}

	return rs.PetsService.UpdatePet(id, patchedPet)
}

We can imagine that c.PatchBody might be able to patch the old structure with different algorithms like RFC6902 according to the given Content-Type

@Seemone
Copy link
Author

Seemone commented Jan 23, 2025

Yes, the use case looks good to me.
The idea of supporting multiple patch formats is great!

@Seemone
Copy link
Author

Seemone commented Jan 23, 2025

you need a structure for every different combination of attributes

No we don't. We just have one PATCH /books with a type BookPatch witch have optional fields, as mentioned above

apologies, I missed the comment in the struct

@dylanhitt
Copy link
Collaborator

Adding a method to ContextWithBody that updates a structure field if there is a corresponding field in the request imho elegantly solves the problem of "PATCH as generic updater".

One thing to note this seems like a pretty expensive operation. This would be quite a bit of reflection.

@dylanhitt
Copy link
Collaborator

I would say that at very least we should warn users, and that if they're looking to optimize they should handle this themselves.

@EwenQuim
Copy link
Member

In the meantime, you can use something like Mergo. If Fuego do have this feature one day, it'll probably be inspired or using this package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Question about Fuego. Not a bug nor a feature clearly identified
Projects
None yet
Development

No branches or pull requests

3 participants