Support the general case of nested structs

[teepark]

22 added support for embedded nested structs, this proposes supporting nested structs in named fields as well.

There is a potential name collision issue if we don't do something about the naming convention:

var Config = struct {
    HTTPClient http.Client
    Timeout int
}{}

func init() {
    envconfig.MustProcess("widget", &Config)
}

Does WIDGET_TIMEOUT correspond to Config.Timeout, or Config.HTTPClient.Timeout?

So I propose we underscore-delimit the field names: WIDGET_TIMEOUT and WIDGET_HTTPCLIENT_TIMEOUT.

envconfig tags would apply globally for nested structs as they do outer structs today (clobbering any prefix), so any field at any position can specify its full env var name. But we can't rely on struct tags alone and still support dropping in a stdlib or third-party-lib struct like the example above.

[evandigby]

In implementing our fork (we needed it today--only reason we forked in spite of existing proposals for solutions), we've found 2 edge cases worth noting to whomever works on the official implementation.

As we flesh out our tests I'll post any more edge cases we may find.

Recursive case

Requires special handling to ensure no infinite recursion.

type Specification struct {
    Recursive *Recursive
}
type Recursive struct {
    Name      string
    Recursive *Recursive
}
var c Specification
Process("env_config", &s)

We proposed requiring all struct fields which will be populated to be explicitly specified. It's a bit of an awkward solution but it was quick and actually made our implementation easier for other reasons.

ENV_CONFIG_RECURSIVE=true # or any value that is non-empty-string
ENV_CONFIG_RECURSIVE_NAME=value
ENV_CONFIG_RECURSIVE_RECURSIVE=true
ENV_CONFIG_RECURSIVE_RECURSIVE_NAME=value

Alternate names

This is touched on by the original issue, but the current support for alternate names seems to dictate that they work with and without prefix. This creates many possible combinations when it comes to struct fields.

type Specification struct {
    StructField *StructField `envconfig:"ALT"`
}
type StructField struct {
    WithAltName string `envconfig:"SUB_ALT"`
}

var c Specification
Process("env_config", &s)

Which are valid? (Which should be?)

ENV_CONFIG_ALT_SUB_ALT=value
ENV_CONFIG_SUB_ALT=value
ALT_SUB_ALT=value
SUB_ALT=value

[teepark]

Hi @evandigby!

Thanks for the thought and care you're obviously putting into this. I think #51 is probably the best of the named struct implementations.

One question: do you actually have the case of a recursively nested struct? Is there another way out of that? It seems like a pretty reasonable limitation for envconfig to not be able to deal with that case (and preferable to requiring some non-empty placeholder environment variable IMO).

[evandigby]

@teepark Thanks!

We don't have the case of a recursive struct. It came out as I was writing the tests ("always test the recursive case" is something I keep in the back of my head). It definitely needs to be handled, as any solution that doesn't will likely trap itself in infinite recursion as my first implementation did.

I see at least 2 other ways to handle the case, I just err'd on the side of more flexible:

1. Arbitrary limit to recursion on structs of the same type (the arbitrary limit could be zero or one).
2. Arbitrary limit as above, but with the bonus of "breaking" the limit with explicit non-empty placeholders.

The side benefit of the non-empty placeholder was it more easily allows the struct field to become just another field handled by process field (with the addition of the full "key" being passed as the prefix recursively), rather than being a special case in the "Process" function. Perhaps this side benefit doesn't outweigh the downside of the placeholders.

[teepark]

On recursion: IMO you're overthinking it. Every time I've used envconfig it was with structs whose only purpose was as a landing place for environment vars, and that seems to be the case for others everywhere I look. So I don't see accidental uses of recursive, self-referential types as a real issue.

I'm gaining clarity now on why other proposals just extended the existing embedded/anonymous support in Process(). That's probably the way to go -- no placeholder variables that way.

[evandigby]

@teepark I have a tendency to overthink, so I don't disagree :) Our use-case is no different than the one you describe. Since this is the type of issue that would certainly be caught with the "I ran it at least once" test, I'm comfortable with it not supporting and flying off into infinity.

Unless someone actually wanted the ability to configure a linked list, a decision tree, etc. I can't imagine a use case for a recursive type. I'll be shocked if I ever see this real-world. Things like that will certainly be better stored/loaded another way.

Do you see benefit in me refactoring mine to fit the simpler paradigm, or is there another pull request ready-to-go that already matches this?

I'm more than happy to do the work, but also don't mind if another PR is ready-to-go.

[teepark]

I've started in on tweaks on top of another PR now, thanks for batting this around with me! :clap:

[teepark]

implemented in #50