laskoviymishka commented on code in PR #2376: URL: https://github.com/apache/iceberg-go/pull/2376#discussion_r4471030940
########## catalog/rest/resttest/doc.go: ########## @@ -0,0 +1,37 @@ +// 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 resttest provides an in-memory Iceberg REST catalog server for +// tests. NewHandler returns an http.Handler that keeps namespaces and table +// pointers in memory and writes table metadata files through the FileIO for +// the configured warehouse, so the catalog/rest client can be driven end to +// end, create namespace through commit and load, without a container: +// +// srv := resttest.NewServer(t) +// cat, err := rest.NewCatalog(ctx, "test", srv.URL) +// +// The server implements the config, namespace, table, rename and register +// endpoints of the REST catalog spec. Commits validate their requirements +// against the current metadata and swap the metadata pointer under a lock, so +// a stale requirement fails with 409 and the client reports +// table.ErrCommitFailed. +// +// Views, multi-table transactions, scan planning, credential vending, metrics +// and authentication are not implemented. They are not advertised in the +// config response, and requests for them return 501. Listing returns a single +// page, and dropping a table with purge does not delete its files. Review Comment: Could the package doc say what a passing run does and doesn't show? As I read it, commits decode into the client's own `table.Requirements`/`table.Updates` ([wire.go](https://github.com/apache/iceberg-go/blob/c19f2bca8f8b19262b042b25042aa5ac24ce3d14/catalog/rest/resttest/wire.go#L94-L98)) and go through the same `Validate` ([utils.go](https://github.com/apache/iceberg-go/blob/c19f2bca8f8b19262b042b25042aa5ac24ce3d14/catalog/internal/utils.go#L270-L279)), so a bug there passes on both sides. Rejected commits other than 409/404 get 400 ([tables.go](https://github.com/apache/iceberg-go/blob/c19f2bca8f8b19262b042b25042aa5ac24ce3d14/catalog/rest/resttest/tables.go#L335-L349)), where Java's `RESTCatalogAdapter` defaults to 500 ([L688](https://github.com/apache/iceberg/blob/apache-iceberg-1.10.0/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java#L688)), so 5xx and unknown-state handling aren't reached. ```suggestion // page, and dropping a table with purge does not delete its files. // // The server decodes commit requirements and updates into the same // table.Requirement and table.Update types the client encodes and checks // them with the same Validate method, so a passing run shows that client // and server agree with each other, not that those JSON shapes or the // validation logic match the spec. Apart from a conflict (409) or a missing // table (404), a rejected commit returns 400, so 5xx responses and unknown // commit state are not exercised. ``` -- 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]
