Fix required in nil child struct

[alecsammon]

I believe this to be a regression somewhere between v0.11.0 and v0.14.0.

If you have a nested structure which is parsed using body=json, then the required tags are being checked for fields of children structs even when they are nil.

I believe in this case it makes sense that the required flag is not checked as the parent struct is nil.

I've included a test that hopefully makes this clearer.

[alecsammon]

Moved to draft - I need to understand order of decoding better!

[ggicci]

Yeah, it was a breaking change introduced by v0.12.0 when I refactored httpin completely. Where I moved the logic of the directives running to a separated package ggicci/owl. In the former implementations, any directives defined in a nested struct will be ignored, for example:

type UpdateUserInput struct {
    Uid     string `in:"path=uid"`
    Payload struct {
        Name string `in:"a=1;b=2;required"` // NOTE: ignored by former versions
        Age  int
    } `in:"body=json"`
}

While since v0.12.0, the directives a=1, b=2 and required defined for field Payload.Name will be executed.

Worth noting here is, currently all the directives will be run in the order they defined in a depth-frist manner. So for the above example, the directives will be run in the following order:

path=uid       (Uid)
body=json      (Payload)
    a=1        (Payload.Name)
    b=2        (Payload.Name)
    required   (Payload.Name)

No matter the body=json returns a nil Payload or not, the directives defined for Payload.Name will run. And when executing the required directive defined for field Payload.Name, it only verifies if the field Payload.Name has been set by its former directives a=1 and b=2. And for the "set" operation here is defined as mutation of the field, whether the final value is an empty/zero value is not considered.

[alecsammon]

Thanks for the really detailed reply - I realised that just after I opened this PR!

I think the way it works now makes much sense - although it does cause a few complications for our use case.

I however think it's probably something that we should manage at our end, instead of in the library.

I might create our own directive that removes all the directives in children structs - this feels a little ugly to be part of this library, but will work well for our needs.

Thanks for your work on this library - it's been really useful for us. Sorry for the wasted time on this PR.

Unrelated - but we've been working on the opposite of this library - httpout - which updates a new request object with the values in the struct based on the same directives.

Is this something of interest - either for us to publish as standalone package, or something to add to this library?

[ggicci]

Hi @alecsammon ,

I think the way it works now makes much sense - although it does cause a few complications for our use case.

I might create our own directive that removes all the directives in children structs - this feels a little ugly to be part of this library, but will work well for our needs.

Actually I don't know how many users really need a support for nested directives either. To be honest, I think the support of nested directives has brought complixity. httpin only supports flat struct before, that's because I believe a flat struct suffices for most usecases, and in my opinion any complicated usecases should consider using a JSON payload instead.

An alternative approach is to make it an optional feature for httpin. Do you think it makes sense?

The new code can look like:

httpin.EnableNestedDirectives()
// or
httpin.DisableNestedDirectives()
// or
httpin.New(ListUsersInput{}, httpin.WithNestedDirectivesEnabled())
// or
httpin.New(ListUsersInput{}, httpin.WithNestedDirectivesDisabled())

Unrelated - but we've been working on the opposite of this library - httpout - which updates a new request object with the values in the struct based on the same directives.

Wow, thanks so much for your effort on this. I'm also working on the same feature these days 😆, on branch feat/encoder. I don't know your roadmap on this and how much it has been implemented. But I'm really happy to know and appreciated! From my side, I plan to release the encoding feature within httpin package in next two days, with a newly added API Core.Encode.

Is this something of interest - either for us to publish as standalone package, or something to add to this library?

I would prefer to have the feature in the same package. Because the resolver tree of a given input struct can be shared between the decoding and encoding function. And this sharing is important because we can ensure the resolver tree used by both are totally the same. If we make a separated package httpout, I'm sure it's better, the naming is more appropriate and especially for users that only need one of these features. But we need to find a way to guarantee any changes to the core should be synced between these two packages as they behave as "brother packages" to some extent. For example, we may need an extra shared package. Apparently it will bring more maintainance burden for such a tiny package.

[alecsammon]

I think having it configurable might be really nice. Either option of enabled by default or disabled by default would work for us. I guess that having it keep the current enabled by default with option to switch off would prevent a breaking change?

The core.Encode option is really exciting - we didn't make very much progress with httpout - we only supported a couple of directives for our specific use case. I'll check out you branch and look forward to deleting my code once it is merged!

I think it makes good sense to have it in the same package - we use it to generate clients for our handlers - so knowing that they work together well would be really important to us. The only reason for the different name for us to was prevent collisions.