zeroshade commented on code in PR #71: URL: https://github.com/apache/terraform-provider-iceberg/pull/71#discussion_r3736927850
########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,242 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package provider + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/table" + "github.com/hashicorp/terraform-plugin-framework/datasource" + dschema "github.com/hashicorp/terraform-plugin-framework/datasource/schema" + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-log/tflog" +) + +var _ datasource.DataSource = &icebergTablesDataSource{} + +func NewTablesDataSource() datasource.DataSource { + return &icebergTablesDataSource{} +} + +type icebergTablesDataSourceModel struct { + ID types.String `tfsdk:"id"` + Namespace types.List `tfsdk:"namespace"` + Tables types.List `tfsdk:"tables"` + Identifiers types.List `tfsdk:"identifiers"` +} + +type icebergTablesDataSource struct { + catalog catalog.Catalog + provider *icebergProvider +} + +func (d *icebergTablesDataSource) Metadata(_ context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_tables" +} + +func (d *icebergTablesDataSource) Schema(_ context.Context, _ datasource.SchemaRequest, resp *datasource.SchemaResponse) { + resp.Schema = dschema.Schema{ + Description: "Lists table names in an Iceberg namespace from the catalog.", + Attributes: map[string]dschema.Attribute{ + "id": dschema.StringAttribute{ + Description: "Dot-separated full namespace identifier.", + Computed: true, + }, + "namespace": dschema.ListAttribute{ + Description: "The namespace to list tables in.", + Required: true, + ElementType: types.StringType, + }, + "tables": dschema.ListAttribute{ + Description: "Table names in the namespace, without namespace segments.", + Computed: true, + ElementType: types.StringType, + }, + "identifiers": dschema.ListAttribute{ + Description: "Dot-separated full table identifiers (namespace segments + table name), matching iceberg_table id format.", + Computed: true, + ElementType: types.StringType, + }, + }, + } +} + +func (d *icebergTablesDataSource) Configure(ctx context.Context, req datasource.ConfigureRequest, resp *datasource.ConfigureResponse) { + if req.ProviderData == nil { + return + } + + provider, ok := req.ProviderData.(*icebergProvider) + if !ok { + resp.Diagnostics.AddError( + "Unexpected Data Source Configure Type", + fmt.Sprintf("Expected *icebergProvider, got: %T. Please report this issue to the provider developers.", req.ProviderData), + ) + + return + } + + d.provider = provider +} + +func (d *icebergTablesDataSource) configureCatalog(ctx context.Context, diags *diag.Diagnostics) { + if d.catalog != nil { + return + } + + if d.provider == nil { + diags.AddError( + "Provider not configured", + "The provider hasn't been configured before this operation", + ) + + return + } + + if d.provider.catalogURI == "" { + return + } + + cat, err := d.provider.NewCatalog(ctx) + if err != nil { + diags.AddError( + "Failed to access catalog", + "Failed to access catalog: "+err.Error(), + ) + + return + } + d.catalog = cat +} + +// identifierString formats a table identifier as a dot-separated string. +// table.Identifier is currently a []string alias with no String() method; +// keep this helper so formatting stays in one place if that changes before v1.0. +func identifierString(ident table.Identifier) string { + return strings.Join(ident, ".") +} + +func sortTableIdentifiers(identifiers []table.Identifier) { + // Stable sort keeps list order deterministic across refreshes so Terraform + // does not report spurious diffs when the catalog returns tables unordered. + slices.SortStableFunc(identifiers, func(a, b table.Identifier) int { + return strings.Compare(identifierString(a), identifierString(b)) + }) +} + +func tableNamesFromIdentifiers(identifiers []table.Identifier) []string { + names := make([]string, 0, len(identifiers)) + for _, ident := range identifiers { + names = append(names, catalog.TableNameFromIdent(ident)) + } + + return names +} + +func tableIdentifierStrings(identifiers []table.Identifier) []string { + out := make([]string, 0, len(identifiers)) + for _, ident := range identifiers { + out = append(out, identifierString(ident)) + } + + return out +} + +func (d *icebergTablesDataSource) Read(ctx context.Context, req datasource.ReadRequest, resp *datasource.ReadResponse) { + tflog.Info(ctx, "Reading iceberg_tables data source") + d.configureCatalog(ctx, &resp.Diagnostics) + if resp.Diagnostics.HasError() { + return + } + + var data icebergTablesDataSourceModel + resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) + if resp.Diagnostics.HasError() { + return + } + + if d.catalog == nil { + resp.Diagnostics.AddError( + "Catalog not available", + "The catalog could not be created (is catalog_uri set?).", + ) + + return + } + + var namespaceName []string + resp.Diagnostics.Append(data.Namespace.ElementsAs(ctx, &namespaceName, false)...) + if resp.Diagnostics.HasError() { + return + } + + if len(namespaceName) == 0 { + resp.Diagnostics.AddError( + "Invalid namespace", + "The namespace attribute must contain at least one namespace segment.", + ) + + return + } + + namespaceIdent := catalog.ToIdentifier(namespaceName...) + + var tableIdents []table.Identifier + for ident, err := range d.catalog.ListTables(ctx, namespaceIdent) { + if err != nil { + if errors.Is(err, catalog.ErrNoSuchNamespace) { Review Comment: **Blocking — error propagation.** This treats any error wrapping `catalog.ErrNoSuchNamespace` as proof the namespace is absent, but in `iceberg-go v0.6.0` the REST catalog wraps *every* page-level HTTP 404 with that sentinel. The dependency's own pagination test yields two valid identifiers and then returns a `NoSuchPageTokenException` 404 — that path would surface here as "No such namespace: <ns>", pointing the user at a namespace that demonstrably exists and hiding the real cause. Good news: the partial-results handling above is already correct — you `return` before writing state, so a truncated list never reaches Terraform. It's only the diagnostic that's wrong. Suggested fix: only claim "Namespace not found" when the namespace is confirmed absent. The cheapest version is to track whether any identifier was successfully yielded before the error, and if so fall through to the generic branch on line 214 so `err.Error()` is preserved: ```go if errors.Is(err, catalog.ErrNoSuchNamespace) && len(tableIdents) == 0 { // genuinely missing namespace } // otherwise: report as "Failed to list tables" with err.Error() ``` Alternatively check namespace existence explicitly before relabeling. Either way it'd be worth a unit test covering "identifiers yielded, then error" — see my note on `Read` coverage. ########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,242 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package provider + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/table" + "github.com/hashicorp/terraform-plugin-framework/datasource" + dschema "github.com/hashicorp/terraform-plugin-framework/datasource/schema" + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-log/tflog" +) + +var _ datasource.DataSource = &icebergTablesDataSource{} + +func NewTablesDataSource() datasource.DataSource { + return &icebergTablesDataSource{} +} + +type icebergTablesDataSourceModel struct { + ID types.String `tfsdk:"id"` + Namespace types.List `tfsdk:"namespace"` + Tables types.List `tfsdk:"tables"` + Identifiers types.List `tfsdk:"identifiers"` +} + +type icebergTablesDataSource struct { + catalog catalog.Catalog + provider *icebergProvider +} + +func (d *icebergTablesDataSource) Metadata(_ context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_tables" +} + +func (d *icebergTablesDataSource) Schema(_ context.Context, _ datasource.SchemaRequest, resp *datasource.SchemaResponse) { + resp.Schema = dschema.Schema{ + Description: "Lists table names in an Iceberg namespace from the catalog.", + Attributes: map[string]dschema.Attribute{ + "id": dschema.StringAttribute{ + Description: "Dot-separated full namespace identifier.", + Computed: true, + }, + "namespace": dschema.ListAttribute{ + Description: "The namespace to list tables in.", + Required: true, + ElementType: types.StringType, + }, + "tables": dschema.ListAttribute{ + Description: "Table names in the namespace, without namespace segments.", + Computed: true, + ElementType: types.StringType, + }, + "identifiers": dschema.ListAttribute{ + Description: "Dot-separated full table identifiers (namespace segments + table name), matching iceberg_table id format.", + Computed: true, + ElementType: types.StringType, + }, + }, + } +} + +func (d *icebergTablesDataSource) Configure(ctx context.Context, req datasource.ConfigureRequest, resp *datasource.ConfigureResponse) { + if req.ProviderData == nil { + return + } + + provider, ok := req.ProviderData.(*icebergProvider) + if !ok { + resp.Diagnostics.AddError( + "Unexpected Data Source Configure Type", + fmt.Sprintf("Expected *icebergProvider, got: %T. Please report this issue to the provider developers.", req.ProviderData), + ) + + return + } + + d.provider = provider +} + +func (d *icebergTablesDataSource) configureCatalog(ctx context.Context, diags *diag.Diagnostics) { + if d.catalog != nil { + return + } + + if d.provider == nil { + diags.AddError( + "Provider not configured", + "The provider hasn't been configured before this operation", + ) + + return + } + + if d.provider.catalogURI == "" { + return + } + + cat, err := d.provider.NewCatalog(ctx) + if err != nil { + diags.AddError( + "Failed to access catalog", + "Failed to access catalog: "+err.Error(), + ) + + return + } + d.catalog = cat +} + +// identifierString formats a table identifier as a dot-separated string. +// table.Identifier is currently a []string alias with no String() method; +// keep this helper so formatting stays in one place if that changes before v1.0. +func identifierString(ident table.Identifier) string { + return strings.Join(ident, ".") +} + +func sortTableIdentifiers(identifiers []table.Identifier) { + // Stable sort keeps list order deterministic across refreshes so Terraform + // does not report spurious diffs when the catalog returns tables unordered. + slices.SortStableFunc(identifiers, func(a, b table.Identifier) int { + return strings.Compare(identifierString(a), identifierString(b)) + }) +} + +func tableNamesFromIdentifiers(identifiers []table.Identifier) []string { + names := make([]string, 0, len(identifiers)) + for _, ident := range identifiers { + names = append(names, catalog.TableNameFromIdent(ident)) + } + + return names +} + +func tableIdentifierStrings(identifiers []table.Identifier) []string { + out := make([]string, 0, len(identifiers)) + for _, ident := range identifiers { + out = append(out, identifierString(ident)) + } + + return out +} + +func (d *icebergTablesDataSource) Read(ctx context.Context, req datasource.ReadRequest, resp *datasource.ReadResponse) { + tflog.Info(ctx, "Reading iceberg_tables data source") + d.configureCatalog(ctx, &resp.Diagnostics) + if resp.Diagnostics.HasError() { + return + } + + var data icebergTablesDataSourceModel + resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) + if resp.Diagnostics.HasError() { + return + } + + if d.catalog == nil { + resp.Diagnostics.AddError( + "Catalog not available", + "The catalog could not be created (is catalog_uri set?).", + ) + + return + } + + var namespaceName []string + resp.Diagnostics.Append(data.Namespace.ElementsAs(ctx, &namespaceName, false)...) + if resp.Diagnostics.HasError() { + return + } + + if len(namespaceName) == 0 { + resp.Diagnostics.AddError( + "Invalid namespace", + "The namespace attribute must contain at least one namespace segment.", + ) + + return + } + + namespaceIdent := catalog.ToIdentifier(namespaceName...) + + var tableIdents []table.Identifier + for ident, err := range d.catalog.ListTables(ctx, namespaceIdent) { + if err != nil { + if errors.Is(err, catalog.ErrNoSuchNamespace) { + resp.Diagnostics.AddError( + "Namespace not found", + "No such namespace: "+identifierString(namespaceIdent), + ) + + return + } + resp.Diagnostics.AddError("failed to list tables", err.Error()) + + return + } + tableIdents = append(tableIdents, ident) + } + + sortTableIdentifiers(tableIdents) Review Comment: **Non-blocking — ordering invariant.** Sorting by the full dot-joined identifier only yields alphabetically-sorted *bare* names when every returned identifier shares the requested namespace prefix. Counterexample: full identifiers `a.zulu` and `b.alpha` sort in that order but project to `tables = ["zulu", "alpha"]`. A conforming REST response only ever returns one namespace here, so real-world output is correct and the two lists stay index-aligned — this isn't a live bug. But the invariant isn't validated in code while `docs/data-sources/tables.md:134` states the guarantee unconditionally. If you want it to hold unconditionally (and to enforce non-recursion defensively), reject empty identifiers and any identifier whose namespace portion differs from `namespaceIdent` before sorting. ########## internal/provider/data_source_tables_test.go: ########## @@ -0,0 +1,344 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package provider + +import ( + "fmt" + "os" + "regexp" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/stretchr/testify/assert" +) + +func TestIdentifierString(t *testing.T) { + t.Parallel() + + assert.Equal(t, "db.events", identifierString(table.Identifier{"db", "events"})) + assert.Equal(t, "analytics.raw.orders", identifierString(table.Identifier{"analytics", "raw", "orders"})) + assert.Equal(t, "", identifierString(nil)) +} + +func TestSortTableIdentifiers(t *testing.T) { + t.Parallel() + + ids := []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + {"ns", "b"}, + {"ns", "a"}, + {"ns", "a"}, + } + sortTableIdentifiers(ids) + + assert.Equal(t, []table.Identifier{ + {"analytics", "raw", "events"}, + {"analytics", "raw", "orders"}, + {"ns", "a"}, + {"ns", "a"}, + {"ns", "b"}, + }, ids) +} + +func TestTableNamesFromIdentifiers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ids []table.Identifier + want []string + }{ + { + name: "empty", + ids: nil, + want: []string{}, + }, + { + name: "single table in flat namespace", + ids: []table.Identifier{{"db", "events"}}, + want: []string{"events"}, + }, + { + name: "preserves caller order", + ids: []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, + want: []string{"orders", "events"}, + }, + { + name: "nested namespace", + ids: []table.Identifier{{"analytics", "prod", "metrics"}}, + want: []string{"metrics"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tableNamesFromIdentifiers(tt.ids)) + }) + } +} + +func TestTableIdentifierStrings(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ids []table.Identifier + want []string + }{ + { + name: "empty", + ids: nil, + want: []string{}, + }, + { + name: "single table in flat namespace", + ids: []table.Identifier{{"db", "events"}}, + want: []string{"db.events"}, + }, + { + name: "preserves caller order", + ids: []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, + want: []string{"analytics.raw.orders", "analytics.raw.events"}, + }, + { + name: "nested namespace", + ids: []table.Identifier{{"analytics", "prod", "metrics"}}, + want: []string{"analytics.prod.metrics"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tableIdentifierStrings(tt.ids)) + }) + } +} + +func TestSortedTableListOutputs(t *testing.T) { + t.Parallel() + + ids := []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + } + sortTableIdentifiers(ids) + + assert.Equal(t, []string{"events", "orders"}, tableNamesFromIdentifiers(ids)) + assert.Equal(t, []string{"analytics.raw.events", "analytics.raw.orders"}, tableIdentifierStrings(ids)) +} + +func TestAccIcebergTablesDataSource_Full(t *testing.T) { + catalogURI := os.Getenv("ICEBERG_CATALOG_URI") + if catalogURI == "" { + t.Skip("ICEBERG_CATALOG_URI not set, skipping tables data source E2E test") + } + + providerCfg := fmt.Sprintf(providerConfig, catalogURI) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccIcebergTablesDataSourceBasicConfig(providerCfg), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.iceberg_tables.read", "namespace.0", "ns_tables_ds_basic"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "id", "ns_tables_ds_basic"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.#", "2"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.0", "alpha_table"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.1", "beta_table"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.#", "2"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.0", "ns_tables_ds_basic.alpha_table"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.1", "ns_tables_ds_basic.beta_table"), + ), + }, + { + Config: testAccIcebergTablesDataSourceNestedConfig(providerCfg), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.iceberg_tables.read", "namespace.0", "analytics"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "namespace.1", "raw"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "id", "analytics.raw"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.#", "1"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.0", "events"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.#", "1"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.0", "analytics.raw.events"), + ), + }, + { + Config: testAccIcebergTablesDataSourceEmptyNamespaceConfig(providerCfg), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.iceberg_tables.read", "namespace.0", "ns_tables_ds_empty"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "tables.#", "0"), + resource.TestCheckResourceAttr("data.iceberg_tables.read", "identifiers.#", "0"), + ), + }, + }, + }) +} + +func TestAccIcebergTablesDataSource_NotFound(t *testing.T) { + catalogURI := os.Getenv("ICEBERG_CATALOG_URI") + if catalogURI == "" { + t.Skip("ICEBERG_CATALOG_URI not set, skipping tables data source E2E test") + } + + providerCfg := fmt.Sprintf(providerConfig, catalogURI) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccIcebergTablesDataSourceMissingConfig(providerCfg), + ExpectError: regexp.MustCompile(`Namespace not found`), + }, + }, + }) +} + +func TestAccIcebergTablesDataSource_EmptyNamespaceAttribute(t *testing.T) { + catalogURI := os.Getenv("ICEBERG_CATALOG_URI") + if catalogURI == "" { + t.Skip("ICEBERG_CATALOG_URI not set, skipping tables data source E2E test") + } + + providerCfg := fmt.Sprintf(providerConfig, catalogURI) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccIcebergTablesDataSourceEmptyNamespaceAttributeConfig(providerCfg), + ExpectError: regexp.MustCompile(`(?s)Invalid namespace.*at least one namespace segment`), + }, + }, + }) +} + +func testAccIcebergTablesDataSourceBasicConfig(providerCfg string) string { + return providerCfg + ` +resource "iceberg_namespace" "db" { + name = ["ns_tables_ds_basic"] +} + +resource "iceberg_table" "alpha" { + namespace = iceberg_namespace.db.name + name = "alpha_table" + schema = { + fields = [ + { + id = 1 + name = "id" + type = "long" + required = true + } + ] + } +} + +resource "iceberg_table" "beta" { + namespace = iceberg_namespace.db.name + name = "beta_table" + schema = { + fields = [ + { + id = 1 + name = "id" + type = "long" + required = true + } + ] + } +} + +data "iceberg_tables" "read" { + namespace = iceberg_namespace.db.name + + depends_on = [ + iceberg_table.alpha, + iceberg_table.beta, + ] +} +` +} + +func testAccIcebergTablesDataSourceNestedConfig(providerCfg string) string { + return providerCfg + ` +resource "iceberg_namespace" "db" { + name = ["analytics", "raw"] Review Comment: **Non-blocking — fixture name collisions.** Successful steps don't leak: later configs drop earlier resources and `resource.Test` destroys at the end. But these hardcoded catalog names can collide with a concurrent run, or with state left behind by an interrupted one. `analytics.raw` here and `definitely_no_such_namespace` at line 341 are the most exposed, being generic enough that another run or another test could plausibly use them. Generating a per-test unique suffix and threading it through the config helpers would make these robust. ########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,242 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package provider + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + "github.com/apache/iceberg-go/catalog" + "github.com/apache/iceberg-go/table" + "github.com/hashicorp/terraform-plugin-framework/datasource" + dschema "github.com/hashicorp/terraform-plugin-framework/datasource/schema" + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-log/tflog" +) + +var _ datasource.DataSource = &icebergTablesDataSource{} + +func NewTablesDataSource() datasource.DataSource { + return &icebergTablesDataSource{} +} + +type icebergTablesDataSourceModel struct { + ID types.String `tfsdk:"id"` + Namespace types.List `tfsdk:"namespace"` + Tables types.List `tfsdk:"tables"` + Identifiers types.List `tfsdk:"identifiers"` +} + +type icebergTablesDataSource struct { + catalog catalog.Catalog + provider *icebergProvider +} + +func (d *icebergTablesDataSource) Metadata(_ context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_tables" +} + +func (d *icebergTablesDataSource) Schema(_ context.Context, _ datasource.SchemaRequest, resp *datasource.SchemaResponse) { + resp.Schema = dschema.Schema{ + Description: "Lists table names in an Iceberg namespace from the catalog.", + Attributes: map[string]dschema.Attribute{ + "id": dschema.StringAttribute{ + Description: "Dot-separated full namespace identifier.", + Computed: true, + }, + "namespace": dschema.ListAttribute{ + Description: "The namespace to list tables in.", + Required: true, + ElementType: types.StringType, + }, + "tables": dschema.ListAttribute{ + Description: "Table names in the namespace, without namespace segments.", Review Comment: **Suggestion — schema/doc wording drift.** The framework descriptions here and on line 75 omit the ordering guarantee that `docs/data-sources/tables.md:134-135` states ("Sorted alphabetically"). Since schema descriptions feed `terraform providers schema -json` and editor tooling, adding the applicable sorting wording here would keep both surfaces telling the same story. (Worth settling alongside the ordering-invariant note on line 221, so the wording matches whatever guarantee you decide to make.) ########## internal/provider/data_source_tables_test.go: ########## @@ -0,0 +1,344 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package provider + +import ( + "fmt" + "os" + "regexp" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/stretchr/testify/assert" +) + +func TestIdentifierString(t *testing.T) { + t.Parallel() + + assert.Equal(t, "db.events", identifierString(table.Identifier{"db", "events"})) + assert.Equal(t, "analytics.raw.orders", identifierString(table.Identifier{"analytics", "raw", "orders"})) + assert.Equal(t, "", identifierString(nil)) +} + +func TestSortTableIdentifiers(t *testing.T) { + t.Parallel() + + ids := []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + {"ns", "b"}, + {"ns", "a"}, + {"ns", "a"}, + } + sortTableIdentifiers(ids) + + assert.Equal(t, []table.Identifier{ + {"analytics", "raw", "events"}, + {"analytics", "raw", "orders"}, + {"ns", "a"}, + {"ns", "a"}, + {"ns", "b"}, + }, ids) +} + +func TestTableNamesFromIdentifiers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ids []table.Identifier + want []string + }{ + { + name: "empty", + ids: nil, + want: []string{}, + }, + { + name: "single table in flat namespace", + ids: []table.Identifier{{"db", "events"}}, + want: []string{"events"}, + }, + { + name: "preserves caller order", + ids: []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, + want: []string{"orders", "events"}, + }, + { + name: "nested namespace", + ids: []table.Identifier{{"analytics", "prod", "metrics"}}, + want: []string{"metrics"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tableNamesFromIdentifiers(tt.ids)) + }) + } +} + +func TestTableIdentifierStrings(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ids []table.Identifier + want []string + }{ + { + name: "empty", + ids: nil, + want: []string{}, + }, + { + name: "single table in flat namespace", + ids: []table.Identifier{{"db", "events"}}, + want: []string{"db.events"}, + }, + { + name: "preserves caller order", + ids: []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, + want: []string{"analytics.raw.orders", "analytics.raw.events"}, + }, + { + name: "nested namespace", + ids: []table.Identifier{{"analytics", "prod", "metrics"}}, + want: []string{"analytics.prod.metrics"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tableIdentifierStrings(tt.ids)) + }) + } +} + +func TestSortedTableListOutputs(t *testing.T) { + t.Parallel() + + ids := []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + } + sortTableIdentifiers(ids) + + assert.Equal(t, []string{"events", "orders"}, tableNamesFromIdentifiers(ids)) + assert.Equal(t, []string{"analytics.raw.events", "analytics.raw.orders"}, tableIdentifierStrings(ids)) +} + +func TestAccIcebergTablesDataSource_Full(t *testing.T) { Review Comment: **Non-blocking — `Read` has no unit coverage.** Everything that exercises config parsing, iterator error handling, and state-setting lives in acceptance tests gated on `ICEBERG_CATALOG_URI`, which the default `go test ./...` gate skips. The helpers are well covered, but `Read` itself — including the pagination-error path I flagged as blocking — isn't tested at all. Worth injecting a fake catalog, or factoring the iterator consumption into a testable helper, and covering: success, empty results, first-yield `ErrNoSuchNamespace`, a generic error, and an error *after* partial results. That last case is the regression test for the blocking issue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
