zeroshade commented on code in PR #71: URL: https://github.com/apache/terraform-provider-iceberg/pull/71#discussion_r3737920645
########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,283 @@ +// 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" + "iter" + "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 +} + +var ( + // errNamespaceNotFound is returned by collectListedTables only when + // ListTables fails with catalog.ErrNoSuchNamespace before yielding any + // identifiers. Mid-pagination 404s that wrap the same catalog sentinel + // must not use this error, so Read can keep the original diagnostic. + errNamespaceNotFound = errors.New("namespace not found") +) + +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. Sorted alphabetically.", + Computed: true, + ElementType: types.StringType, + }, + "identifiers": dschema.ListAttribute{ + Description: "Dot-separated full table identifiers (namespace segments + table name), matching iceberg_table id format. Sorted alphabetically.", + 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 +} + +// collectListedTables consumes a ListTables iterator, enforcing that every +// yielded identifier belongs to namespaceIdent (non-recursive listing). +// +// ErrNoSuchNamespace is only treated as a missing namespace when no identifiers +// were yielded first. In iceberg-go's REST catalog, page-level HTTP 404s are +// also wrapped with that sentinel, so a failure after partial results must +// preserve the original error instead of reporting "Namespace not found". +func collectListedTables(seq iter.Seq2[table.Identifier, error], namespaceIdent table.Identifier) ([]table.Identifier, error) { + var tableIdents []table.Identifier + for ident, err := range seq { + if err != nil { + if errors.Is(err, catalog.ErrNoSuchNamespace) && len(tableIdents) == 0 { + return nil, fmt.Errorf("%w: %s", errNamespaceNotFound, identifierString(namespaceIdent)) + } Review Comment: **Blocking — the discriminator still has a reachable hole.** Thank you for restructuring this; keying `Read` off a distinct `errNamespaceNotFound` is exactly right. But `len(tableIdents) == 0` isn't equivalent to "the first page failed," which is what the logic needs. Look at the pinned client's pagination loop (`catalog/rest/rest.go`, `ListTables`): ```go for { tables, nextPageToken, err := r.listTablesPage(ctx, namespace, pageToken, pageSize) if err != nil { yield(table.Identifier{}, err) return } for _, tbl := range tables { // zero iterations when the page is empty if !yield(tbl, nil) { return } } if nextPageToken == "" { // the ONLY success-termination condition return } pageToken = nextPageToken } ``` There's no "page returned zero rows ⇒ stop" check — termination depends *solely* on an empty `next-page-token`. So this sequence is producible: 1. Page 1 → HTTP 200, `{"identifiers": [], "next-page-token": "tok"}` — yields **nothing** 2. Page 2 → HTTP 404 — yields the error The consumer's first observation is an `ErrNoSuchNamespace`-wrapping error with `len(tableIdents) == 0`, so this branch fires and reports "Namespace not found" for a namespace that demonstrably exists — the precise failure mode the comment above says it prevents. I confirmed this against the pinned client with an `httptest` server: ``` identifiers yielded before error: 0 ([]) error: NoSuchPageTokenException: Token expired or invalid errors.Is(err, catalog.ErrNoSuchNamespace) = true >>> guard `len==0 && errors.Is(ErrNoSuchNamespace)` fires = true ``` Worth stressing *why* the sentinel is ambiguous in the first place: `listTablesPage` passes `map[int]error{http.StatusNotFound: catalog.ErrNoSuchNamespace}`, and `handleNon200` selects the sentinel **by HTTP status code alone**. The response body's `type` field is decoded but never consulted for that choice. So *every* 404 on *any* page — expired page token, namespace dropped mid-scan, proxy 404 — arrives as `ErrNoSuchNamespace`. And an empty non-final page isn't server misbehavior: the REST spec doesn't require `identifiers` to be non-empty when a continuation token is present. A server that filters *after* fetching a page-sized batch (authorization filtering, tombstone skipping) produces exactly this shape. Pair it with a short-lived page token — the very thing `NoSuchPageTokenException` exists for — and the two-step is realistic, if uncommon. The iterator doesn't expose page boundaries, so yield count can't distinguish these cases even in principle. Probe existence explicitly on the error path instead: ```go // catalog.Catalog interface method — a single HEAD /v1/namespaces/{ns}, // maps 404 to (false, nil) and preserves any other error. CheckNamespaceExists(ctx context.Context, namespace table.Identifier) (bool, error) ``` On an ambiguous `ErrNoSuchNamespace`, ask the catalog whether the namespace exists and let *that* decide between `errNamespaceNotFound` and passing the raw error through. It costs one extra round trip only on the error path, it's on the `Catalog` interface so the provider stays catalog-agnostic, and the discrimination becomes exact rather than heuristic. That does mean `collectListedTables` needs the catalog (or a small existence-checking func) passed in — happy to look at whatever shape you prefer. Please also add the empty-first-page regression test: yields `[]`, then a `NoSuchPageTokenException`-style error wrapping `catalog.ErrNoSuchNamespace`, asserting the result is **not** `errNamespaceNotFound`. Note that upstream's own `TestListTablesPaginationErrorOnSubsequentPage` is a near-miss here — it uses a non-empty first page, which is the one configuration where the current guard happens to behave correctly. ########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,283 @@ +// 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" + "iter" + "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 +} + +var ( Review Comment: **Blocking — `gofumpt` failure (CI gate).** `gofumpt` collapses a single-declaration parenthesized `var` block. `.golangci.yml` enables it under `formatters`, so `golangci-lint run` fails here: ``` internal/provider/data_source_tables.go:53:1: File is not properly formatted (gofumpt) ``` The fix (from `gofumpt -d`) — move the comment above and drop the parens: ```go // errNamespaceNotFound is returned by collectListedTables only when // ListTables fails with catalog.ErrNoSuchNamespace before yielding any // identifiers. Mid-pagination 404s that wrap the same catalog sentinel // must not use this error, so Read can keep the original diagnostic. var errNamespaceNotFound = errors.New("namespace not found") ``` (Running `gofumpt -w internal/provider/data_source_tables.go` does it for you. Plain `gofmt` doesn't catch this, which is probably why it slipped through.) ########## internal/provider/data_source_tables.go: ########## @@ -0,0 +1,283 @@ +// 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" + "iter" + "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 +} + +var ( + // errNamespaceNotFound is returned by collectListedTables only when + // ListTables fails with catalog.ErrNoSuchNamespace before yielding any + // identifiers. Mid-pagination 404s that wrap the same catalog sentinel + // must not use this error, so Read can keep the original diagnostic. + errNamespaceNotFound = errors.New("namespace not found") +) + +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. Sorted alphabetically.", + Computed: true, + ElementType: types.StringType, + }, + "identifiers": dschema.ListAttribute{ + Description: "Dot-separated full table identifiers (namespace segments + table name), matching iceberg_table id format. Sorted alphabetically.", + 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 +} + +// collectListedTables consumes a ListTables iterator, enforcing that every +// yielded identifier belongs to namespaceIdent (non-recursive listing). +// +// ErrNoSuchNamespace is only treated as a missing namespace when no identifiers +// were yielded first. In iceberg-go's REST catalog, page-level HTTP 404s are +// also wrapped with that sentinel, so a failure after partial results must +// preserve the original error instead of reporting "Namespace not found". +func collectListedTables(seq iter.Seq2[table.Identifier, error], namespaceIdent table.Identifier) ([]table.Identifier, error) { + var tableIdents []table.Identifier + for ident, err := range seq { + if err != nil { + if errors.Is(err, catalog.ErrNoSuchNamespace) && len(tableIdents) == 0 { + return nil, fmt.Errorf("%w: %s", errNamespaceNotFound, identifierString(namespaceIdent)) + } + + return nil, err + } + + if len(ident) == 0 { + return nil, fmt.Errorf("catalog returned empty table identifier") Review Comment: **Blocking — `perfsprint` failure (CI gate).** `fmt.Errorf` with no format arguments should be `errors.New`. `perfsprint` is enabled in `.golangci.yml`: ``` internal/provider/data_source_tables.go:192:16: error-format: fmt.Errorf can be replaced with errors.New (perfsprint) ``` ```go return nil, errors.New("catalog returned empty table identifier") ``` The `fmt.Errorf` calls just below it are fine — they have real format args. While you're here: this guard being *before* the `catalog.NamespaceFromIdent` call on the next line is load-bearing, since that helper is `ident[:len(ident)-1]` with no bounds check and panics on an empty identifier. Worth a brief note in the comment so a future refactor doesn't reorder them. ########## internal/provider/data_source_tables_test.go: ########## @@ -0,0 +1,464 @@ +// 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 ( + "errors" + "fmt" + "iter" + "os" + "regexp" + "testing" + + "github.com/apache/iceberg-go/catalog" + "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 listTablesSeq(yields []table.Identifier, finalErr error) iter.Seq2[table.Identifier, error] { + return func(yield func(table.Identifier, error) bool) { + for _, ident := range yields { + if !yield(ident, nil) { + return + } + } + if finalErr != nil { + yield(table.Identifier{}, finalErr) + } + } +} + +func TestCollectListedTables(t *testing.T) { + t.Parallel() + + ns := table.Identifier{"analytics", "raw"} + + t.Run("success", func(t *testing.T) { + t.Parallel() + + got, err := collectListedTables(listTablesSeq([]table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, nil), ns) + if !assert.NoError(t, err) { + return + } + assert.Equal(t, []table.Identifier{ + {"analytics", "raw", "orders"}, + {"analytics", "raw", "events"}, + }, got) + }) + + t.Run("empty results", func(t *testing.T) { + t.Parallel() + + got, err := collectListedTables(listTablesSeq(nil, nil), ns) + if !assert.NoError(t, err) { + return + } + assert.Empty(t, got) + }) + + t.Run("first yield ErrNoSuchNamespace", func(t *testing.T) { + t.Parallel() + + got, err := collectListedTables(listTablesSeq(nil, catalog.ErrNoSuchNamespace), ns) + assert.Nil(t, got) + if !assert.Error(t, err) { + return + } + assert.ErrorIs(t, err, errNamespaceNotFound) + assert.Contains(t, err.Error(), "analytics.raw") + // Catalog sentinel is intentionally not wrapped here; Read keys off + // errNamespaceNotFound so mid-pagination ErrNoSuchNamespace stays distinct. + assert.False(t, errors.Is(err, catalog.ErrNoSuchNamespace)) + }) + + t.Run("generic error", func(t *testing.T) { + t.Parallel() + + wantErr := errors.New("catalog unavailable") + got, err := collectListedTables(listTablesSeq(nil, wantErr), ns) + assert.Nil(t, got) + assert.ErrorIs(t, err, wantErr) + }) + + t.Run("error after partial results preserves cause", func(t *testing.T) { + t.Parallel() + + // Mimics iceberg-go REST pagination: identifiers from page 1, then a + // page-level HTTP 404 wrapped as ErrNoSuchNamespace (e.g. bad page token). + pageErr := fmt.Errorf("NoSuchPageTokenException: %w", catalog.ErrNoSuchNamespace) + got, err := collectListedTables(listTablesSeq([]table.Identifier{ + {"analytics", "raw", "events"}, + {"analytics", "raw", "orders"}, + }, pageErr), ns) + assert.Nil(t, got) + if !assert.Error(t, err) { + return + } + assert.ErrorIs(t, err, catalog.ErrNoSuchNamespace) + assert.False(t, errors.Is(err, errNamespaceNotFound)) + assert.Contains(t, err.Error(), "NoSuchPageTokenException") + }) + + t.Run("rejects empty identifier", func(t *testing.T) { + t.Parallel() + + got, err := collectListedTables(listTablesSeq([]table.Identifier{{}}, nil), ns) + assert.Nil(t, got) + if !assert.Error(t, err) { + return + } + assert.Contains(t, err.Error(), "empty table identifier") + }) + + t.Run("rejects identifier outside namespace", func(t *testing.T) { + t.Parallel() + + got, err := collectListedTables(listTablesSeq([]table.Identifier{ + {"other", "ns", "events"}, + }, nil), ns) + assert.Nil(t, got) + if !assert.Error(t, err) { + return + } + assert.Contains(t, err.Error(), `outside requested namespace "analytics.raw"`) + }) +} + +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) + suffix := resource.UniqueId() Review Comment: **Blocking — `staticcheck` SA1019 (CI gate).** `resource.UniqueId()` is deprecated in `terraform-plugin-testing` v1.15.0 ("Copy this function to the provider codebase or use `helper/id.Unique`"). `.github/workflows/go-ci.yml` runs `staticcheck ./...` unconditionally, so this fails CI at this line and at line 328: ``` internal/provider/data_source_tables_test.go:275:12: resource.UniqueId is deprecated ... (SA1019) internal/provider/data_source_tables_test.go:328:41: resource.UniqueId is deprecated ... (SA1019) ``` The collision-safety idea is right — just needs a non-deprecated source. `helper/acctest` in the same (already-direct) module isn't deprecated: ```go import "github.com/hashicorp/terraform-plugin-testing/helper/acctest" suffix := acctest.RandString(8) // or strconv.Itoa(acctest.RandInt()) ``` Note `acctest.RandomWithPrefix` joins with a **hyphen**, which may not be valid in a namespace name for every catalog — so prefer building the name yourself with an underscore as you're doing now. The other option is `terraform-plugin-sdk/v2/helper/id.Unique`, but that'd promote the SDK from an indirect to a direct dependency just for test naming; `acctest` avoids that. -- 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]
