laskoviymishka commented on code in PR #2012:
URL: https://github.com/apache/iceberg-go/pull/2012#discussion_r4024089789
##########
catalog/glue/glue.go:
##########
@@ -653,6 +653,11 @@ func (c *Catalog) CreateNamespace(ctx context.Context,
namespace table.Identifie
DatabaseInput: constructDatabaseInput(database, props),
})
if err != nil {
+ var alreadyExistsErr *types.AlreadyExistsException
+ if errors.As(err, &alreadyExistsErr) {
+ return fmt.Errorf("failed to create database %s: %w",
database, catalog.ErrNamespaceAlreadyExists)
Review Comment:
This mapping is right, but the same `AlreadyExistsException` comes back from
`CreateTable`, `RegisterTable`, and the table-not-found create path in
`CommitTable`, and none of those map it to `ErrTableAlreadyExists`: the raw
Glue exception gets wrapped in a plain `fmt.Errorf`, so `errors.Is(err,
catalog.ErrTableAlreadyExists)` is false for Glue while hive, sql, and rest all
map it.
The same `errors.As` pattern would cover all three. Not strictly this PR's
scope, so extending here or filing a follow-up both work. wdyt?
##########
catalog/glue/glue_test.go:
##########
@@ -1029,6 +1029,35 @@ func TestGlueCreateNamespace(t *testing.T) {
assert.NoError(err)
}
+func TestGlueCreateNamespaceAlreadyExists(t *testing.T) {
+ assert := require.New(t)
+
+ mockGlueSvc := &mockGlueClient{}
+
+ mockGlueSvc.On("CreateDatabase", mock.Anything,
&glue.CreateDatabaseInput{
+ DatabaseInput: &types.DatabaseInput{
+ Name: aws.String("test_namespace"),
+ Description: aws.String("Test Description"),
+ LocationUri: aws.String("s3://test-location"),
+ Parameters: map[string]string{},
+ },
+ }, mock.Anything).Return(&glue.CreateDatabaseOutput{},
&types.AlreadyExistsException{
+ Message: aws.String("Database already exists"),
+ }).Once()
+
+ glueCatalog := &Catalog{
+ glueSvc: mockGlueSvc,
+ }
+
+ props := map[string]string{
+ "comment": "Test Description",
+ PropsKeyLocation: "s3://test-location",
+ }
+
+ err := glueCatalog.CreateNamespace(context.TODO(),
DatabaseIdentifier("test_namespace"), props)
+ assert.ErrorIs(err, catalog.ErrNamespaceAlreadyExists)
Review Comment:
This proves the remap, but the branch has a second behavior (every
non-`AlreadyExists` error has to pass through untouched), and nothing covers
it. A regression that hoisted the `errors.As` block or dropped the early return
would still leave this green.
I'd add a sibling test that returns a plain error (or
`AccessDeniedException`) and asserts `errors.Is(err,
catalog.ErrNamespaceAlreadyExists)` is false with the original still in the
chain. While you're in here, `assert.ErrorContains(err, "test_namespace")` pins
the message shape either way.
##########
catalog/glue/glue_test.go:
##########
@@ -1029,6 +1029,35 @@ func TestGlueCreateNamespace(t *testing.T) {
assert.NoError(err)
}
+func TestGlueCreateNamespaceAlreadyExists(t *testing.T) {
+ assert := require.New(t)
+
+ mockGlueSvc := &mockGlueClient{}
+
+ mockGlueSvc.On("CreateDatabase", mock.Anything,
&glue.CreateDatabaseInput{
+ DatabaseInput: &types.DatabaseInput{
+ Name: aws.String("test_namespace"),
+ Description: aws.String("Test Description"),
+ LocationUri: aws.String("s3://test-location"),
+ Parameters: map[string]string{},
+ },
+ }, mock.Anything).Return(&glue.CreateDatabaseOutput{},
&types.AlreadyExistsException{
+ Message: aws.String("Database already exists"),
+ }).Once()
Review Comment:
The `.Once()` here doesn't actually assert anything without a
`mockGlueSvc.AssertExpectations(t)` at the end: right now it's documentation,
and the test would still pass if `CreateDatabase` were never called. Most of
the `.Once()` tests in this file (`TestGlueDropTable`,
`TestGlueListTablesError`, `TestGlueDropNamespace`) close with it.
`TestGlueCreateNamespace` omits it too, so I'd add it to both.
--
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]
