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

fix: update (hotels&airports) #78

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: update (hotels&airports) #78

wants to merge 6 commits into from

Conversation

led0nk
Copy link
Collaborator

@led0nk led0nk commented Apr 7, 2024

Seems like while iterating through airports & hotels they got the model.Location assigned.
But the form-data is only unmarshalled into the model.
Therefore it doesn't get assigned with the correct uuid, but instead gets the uuid of the empty model.Location.

Just additionally assigning should solve this problem, otherwise you could also parse the UUID directly into the model.Location (line 857).

@led0nk led0nk requested a review from frzifus April 7, 2024 21:39
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I assume it makes more sense to set an ID tag name here:
https://github.com/frzifus/lets-party/blob/4ffa29606aef281942efea4a048d3d51b4bbe659/intern/model/guest.go#L40

I remember that the ID tag was removed because the unmarshal function did panic.

Maybe we'll apply this change as a "hotfix" and add some tests for the form.Unmarshal function later?

@led0nk
Copy link
Collaborator Author

led0nk commented Apr 9, 2024

Just an update:

As requested i added the comment.
On top of that i extended Unmarshal() by "reflect.UUID-type" and created form_test.go to test Unmarshal().
Turns out problem isn't the reflect of UUID but the reflect.Slice as it has problems with any slices which aren't of type string/slice/struct -> which again have to consist of type string.

I'm trying to fix this the next few days.

Comment on lines 98 to 104
case reflect.TypeOf(uuid.UUID{}).Kind():
if fieldValRaw == "" {
continue
}
parsedUUID := uuid.MustParse(fieldValRaw)

fieldVal.Set(reflect.ValueOf(parsedUUID))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, but that would mean we have to implement any new type definition?

Since uuid.UUID is a [16]byte, can we maybe change the case to check for reflect.Array instead?


And in case that does not work, we could maybe provide a way to extend the Unmarshal capabilities by providing a custom HtmlFormUnmarshaler implementation.

case reflect.TypeOf((*HtmlFormUnmarshaler)(nil)).Elem().Kind():
    if fieldValRaw == "" {
        continue
    }
    fieldValue := reflect.New(field.Type).Interface().(HtmlFormUnmarshaler)
    if err := fieldValue.UnmarshalFormValue(fieldValRaw); err != nil {
        return err
    }
    fieldVal.Set(reflect.ValueOf(fieldValue).Elem())

In that case uuid.UUID has to implement this method:

type HtmlFormUnmarshaler interface {
    UnmarshalFormValue(value string) error
}

For this we could wrap the wrap the type:

type UUID struct {
    uuid.UUID
}
func (u *UUID) UnmarshalFormValue(value string) error {
    panic("not implemented")
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reflect.Array seems to work fine here

@led0nk led0nk requested a review from frzifus April 10, 2024 17:15
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.

Updating hotels & airports does not work
2 participants