hashicorp / terraform-plugin-framework

A next-generation framework for building Terraform providers.
https://developer.hashicorp.com/terraform/plugin/framework
Mozilla Public License 2.0
302 stars 93 forks source link

Support `Default` in Data Source Schema #751

Open zliang-akamai opened 1 year ago

zliang-akamai commented 1 year ago

Similar to the default support in the resource, https://developer.hashicorp.com/terraform/plugin/framework/resources/default

This is useful because some data sources may have more than one (id) arguments in the config.

bflad commented 1 year ago

Hi @zliang-akamai 👋 Thank you for raising this feature request.

To provide some additional context here, which I do not believe was copied from our internal design documentation into #668:

Data sources and providers are considered out of scope because Terraform does not have the notion of planning for those concepts. While technically possible to implement similar default value handlers on those concepts, the framework would then need to implement its own details of how to surface that data, which goes against its design principle of surfacing abstractions concisely. For example, it would be incorrect to overwrite null configuration data with default values because provider developers could no longer determine the source of the configuration data. It would also be confusing for provider developers if the merged configuration and default values data was surfaced in a separate data field, which is not present in the protocol. They would need to choose when to use the separate data field, which is error prone.

Because of these considerations, there is some initial hesitation to introducing the abstraction. If some of the design details can be worked out without going against prior design decisions in the framework, we can certainly entertain proposals.

drewmullen commented 12 months ago

+1 it would be nice to get this feature for data sources similar to resources: https://github.com/hashicorp/terraform-plugin-framework/blob/main/resource/schema/string_attribute.go#L154

ATM I am writing a data source that can search for a resource a number of ways. The AWS API only has a list function and also includes a status parameter which... in every conceivable circumstance will be ACTIVE. However, the API allows for searching for an INACTIVE status also so I'd like to include it with a sane default

bflad commented 12 months ago

@drewmullen you can default the value in the data source Read method and check if the attribute value in Config is not null to overwrite that value.

Until there's a reasonable discussion about https://github.com/hashicorp/terraform-plugin-framework/issues/751#issuecomment-1585145090, I wouldn't anticipate this feature request to move forward. It's not that we don't think exposing this sort of functionality is not valuable, its that exposing it is awkward against the design of the framework.

magodo commented 11 months ago

Hi @bflad

From #668, it says:

The framework would handle the semantics of only consulting the default value handling when the configuration is null.

Isn't this the same case for data source when its O+C attribute is nil in the configuration, then the default should be applied and accessable from the req.Config?

Apologize if I missed anything, as I didn't quite follow your quotation:

For example, it would be incorrect to overwrite null configuration data with default values because provider developers could no longer determine the source of the configuration data.

What is the case when users want to determine an attribute in the req.Config comes from default or configuration? Or it has special meaning to data source?

austinvalle commented 1 month ago

Here's a quick attempt to put the ideas that are circulating around this issue more concretely...


Both would start with a similar schema definition in data sources to managed resource defaults:

resp.Schema = schema.Schema{
    Attributes: map[string]schema.Attribute{
        "name": schema.StringAttribute{
            Default:  stringdefault.StaticString("austin"),
            Optional: true,
            Computed: true,
        },
    },
}

Idea 1 - New framework-generated data objects for datasource.Read

The datasource.ReadRequest would receive a new data field that would be generated by terraform-plugin-framework before datasource.Read is called, first copying the configuration over, then applying the default values to it:

package datasource

type ReadRequest struct {
    // Config is the configuration the user supplied for the data source.
    Config tfsdk.Config

    // .. other existing fields

    // FrameworkProposedState is the result of merging the user configuration with
    // provider-defined defaults. This field is pre-populated from ReadRequest.Config.
    //
    // Note: naming is hard, so just throwing something bad out first, then can adjust later
    FrameworkProposedState tfsdk.State
}

You'd then have to access it as a separate/new field in your Read method:

func (r *thingDataSource) Read(ctx context.Context, req datasource.ReadRequest, resp *datasource.ReadResponse) {
    var data thingDataSourceModel
    resp.Diagnostics.Append(req.FrameworkProposedState.Get(ctx, &data)...)

    if resp.Diagnostics.HasError() {
        return
    }

    resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) // data.Name == "austin" if not configured
}

Pros

Cons

Idea 2 - New method on tfsdk.Config for retrieving data with defaults

We could keep the request/response the same, and just provide a new method for applying defaults to a configuration (tentatively producing a tfsdk.State object):

func (r *thingDataSource) Read(ctx context.Context, req datasource.ReadRequest, resp *datasource.ReadResponse) {
    dataWithDefaults, diags := req.Config.ApplyDefaults(ctx) // use the schema to apply defaults to computed values with `null` config values
    resp.Diagnostics.Append(diags...)
    if resp.Diagnostics.HasError() {
        // Error applying defaults!
        return
    }

    var data thingDataSourceModel
    resp.Diagnostics.Append(dataWithDefaults.Get(ctx, &data)...)

    if resp.Diagnostics.HasError() {
        return
    }

    resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) // data.Name == "austin" if not configured
}

Pros

Cons


Both ideas could be potentially applied to provider configurations as well, although there introduces additional confusion that provider instances don't have any state during Configure, so we may have to name them something different. Provider schemas also don't support Computed so you have another awkward difference as well 🙁

In terms of potential other proposals, I agree that a main goal would be to ensure that we clearly distinguish between protocol provided data and framework provided data, and avoid conflating the two. So I'd say I'm of the mind that overwriting (datasource.ReadRequest).Config with default values prior to Read should be avoided as a solution.