Skip to content

feat: move import database metadata to metadata field - #1804

Merged
karakanb merged 3 commits into
mainfrom
sabrikaragonen/import-metadata-field
Aug 27, 2026
Merged

feat: move import database metadata to metadata field#1804
karakanb merged 3 commits into
mainfrom
sabrikaragonen/import-metadata-field

Conversation

@sabrikaragonen

@sabrikaragonen sabrikaragonen commented Mar 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Moves operational metadata (row count, size, and timestamps) from the asset description field to the structured metadata field in bruin import database
  • Keeps the database table comment in description and writes the database owner to the dedicated owner field
  • Refreshes import-generated metadata on re-import, removes stale generated keys, and preserves custom metadata
  • Applies the same behavior to command-line, interactive TUI, and --ingestr imports
  • Updates the database import documentation with the generated asset structure

Test plan

  • make format
  • make test
  • GitHub CI Checks workflow
  • Manual: run bruin import database and verify YAML output has a metadata block instead of operational data embedded in description
  • Manual: re-run import on the same asset and verify generated metadata is refreshed while custom metadata remains

@greptile-apps

greptile-apps Bot commented Mar 13, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
This is a comment left during a code review.
Path: cmd/import.go
Line: 312-321

Comment:
**Stale import-generated metadata keys after re-import**

The merge loop only adds or overwrites keys that exist in `createdAsset.Metadata`; it never removes keys that were previously written by an import but are no longer returned by the DB. This directly contradicts the PR's stated goal: "On re-import, metadata is now always refreshed."

Concrete scenario:
1. First import: `RowCount` is available → `existingAsset.Metadata["row_count"] = "1 000 000"`.
2. Second import: DB no longer reports `RowCount` (stats dropped, permissions change, etc.) → `createdAsset.Metadata` has no `row_count` key.
3. After the merge loop the existing `row_count: "1 000 000"` stays untouched — permanently stale.

The same applies to `created_at`, `last_modified`, and `size`.

One way to fix this is to explicitly delete the known import-generated keys from `existingAsset.Metadata` before applying the merge, so absent metrics are cleared rather than left behind:

```go
// Always refresh metadata so it stays current on re-import.
// We merge key-by-key to preserve any user-added custom metadata keys.
if createdAsset.Metadata != nil {
    if existingAsset.Metadata == nil {
        existingAsset.Metadata = make(pipeline.EmptyStringMap)
    }
    // Clear import-generated keys first so absent metrics don't linger.
    for _, k := range []string{"extracted_at", "created_at", "last_modified", "row_count", "size"} {
        delete(existingAsset.Metadata, k)
    }
    for k, v := range createdAsset.Metadata {
        existingAsset.Metadata[k] = v
    }
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cmd/import_test.go
Line: 341-352

Comment:
**Test duplicates production logic instead of exercising it**

`TestReimportUpdatesMetadata` manually copies the exact merge loop from `runImport()` rather than calling `runImport()` (or a helper extracted from it). This means if the merge logic in production changes, this test will continue to pass even though the behaviour is now wrong — it's testing a stale copy of the code, not the code itself.

Consider either:
- Extracting the merge logic into a package-level helper (e.g. `mergeAssetMetadata`) that both `runImport` and the test call, or
- Exercising the real `runImport()` path with a filesystem-backed test (even an in-memory `afero.MemMapFs`).

As written, the test provides false confidence that the re-import path works correctly.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: f262243

Comment thread cmd/import.go Outdated
Comment on lines +312 to +321
// Always refresh metadata so it stays current on re-import.
// We merge key-by-key to preserve any user-added custom metadata keys.
if createdAsset.Metadata != nil {
if existingAsset.Metadata == nil {
existingAsset.Metadata = make(pipeline.EmptyStringMap)
}
for k, v := range createdAsset.Metadata {
existingAsset.Metadata[k] = v
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale import-generated metadata keys after re-import

The merge loop only adds or overwrites keys that exist in createdAsset.Metadata; it never removes keys that were previously written by an import but are no longer returned by the DB. This directly contradicts the PR's stated goal: "On re-import, metadata is now always refreshed."

Concrete scenario:

  1. First import: RowCount is available → existingAsset.Metadata["row_count"] = "1 000 000".
  2. Second import: DB no longer reports RowCount (stats dropped, permissions change, etc.) → createdAsset.Metadata has no row_count key.
  3. After the merge loop the existing row_count: "1 000 000" stays untouched — permanently stale.

The same applies to created_at, last_modified, and size.

One way to fix this is to explicitly delete the known import-generated keys from existingAsset.Metadata before applying the merge, so absent metrics are cleared rather than left behind:

// Always refresh metadata so it stays current on re-import.
// We merge key-by-key to preserve any user-added custom metadata keys.
if createdAsset.Metadata != nil {
    if existingAsset.Metadata == nil {
        existingAsset.Metadata = make(pipeline.EmptyStringMap)
    }
    // Clear import-generated keys first so absent metrics don't linger.
    for _, k := range []string{"extracted_at", "created_at", "last_modified", "row_count", "size"} {
        delete(existingAsset.Metadata, k)
    }
    for k, v := range createdAsset.Metadata {
        existingAsset.Metadata[k] = v
    }
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/import.go
Line: 312-321

Comment:
**Stale import-generated metadata keys after re-import**

The merge loop only adds or overwrites keys that exist in `createdAsset.Metadata`; it never removes keys that were previously written by an import but are no longer returned by the DB. This directly contradicts the PR's stated goal: "On re-import, metadata is now always refreshed."

Concrete scenario:
1. First import: `RowCount` is available → `existingAsset.Metadata["row_count"] = "1 000 000"`.
2. Second import: DB no longer reports `RowCount` (stats dropped, permissions change, etc.) → `createdAsset.Metadata` has no `row_count` key.
3. After the merge loop the existing `row_count: "1 000 000"` stays untouched — permanently stale.

The same applies to `created_at`, `last_modified`, and `size`.

One way to fix this is to explicitly delete the known import-generated keys from `existingAsset.Metadata` before applying the merge, so absent metrics are cleared rather than left behind:

```go
// Always refresh metadata so it stays current on re-import.
// We merge key-by-key to preserve any user-added custom metadata keys.
if createdAsset.Metadata != nil {
    if existingAsset.Metadata == nil {
        existingAsset.Metadata = make(pipeline.EmptyStringMap)
    }
    // Clear import-generated keys first so absent metrics don't linger.
    for _, k := range []string{"extracted_at", "created_at", "last_modified", "row_count", "size"} {
        delete(existingAsset.Metadata, k)
    }
    for k, v := range createdAsset.Metadata {
        existingAsset.Metadata[k] = v
    }
}
```

How can I resolve this? If you propose a fix, please make it concise.

Comment thread cmd/import_test.go Outdated
Comment on lines +341 to +352
// Apply the same merge logic used in the re-import branch
if createdAsset.Metadata != nil {
if existingAsset.Metadata == nil {
existingAsset.Metadata = make(pipeline.EmptyStringMap)
}
for k, v := range createdAsset.Metadata {
existingAsset.Metadata[k] = v
}
}
if createdAsset.Owner != "" {
existingAsset.Owner = createdAsset.Owner
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test duplicates production logic instead of exercising it

TestReimportUpdatesMetadata manually copies the exact merge loop from runImport() rather than calling runImport() (or a helper extracted from it). This means if the merge logic in production changes, this test will continue to pass even though the behaviour is now wrong — it's testing a stale copy of the code, not the code itself.

Consider either:

  • Extracting the merge logic into a package-level helper (e.g. mergeAssetMetadata) that both runImport and the test call, or
  • Exercising the real runImport() path with a filesystem-backed test (even an in-memory afero.MemMapFs).

As written, the test provides false confidence that the re-import path works correctly.

Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/import_test.go
Line: 341-352

Comment:
**Test duplicates production logic instead of exercising it**

`TestReimportUpdatesMetadata` manually copies the exact merge loop from `runImport()` rather than calling `runImport()` (or a helper extracted from it). This means if the merge logic in production changes, this test will continue to pass even though the behaviour is now wrong — it's testing a stale copy of the code, not the code itself.

Consider either:
- Extracting the merge logic into a package-level helper (e.g. `mergeAssetMetadata`) that both `runImport` and the test call, or
- Exercising the real `runImport()` path with a filesystem-backed test (even an in-memory `afero.MemMapFs`).

As written, the test provides false confidence that the re-import path works correctly.

How can I resolve this? If you propose a fix, please make it concise.

sabrikaragonen and others added 3 commits August 26, 2026 16:58
The `bruin import database` command previously embedded operational metadata
(row count, size, timestamps, owner) into the asset Description field. This
caused metadata to become stale on re-import since existing descriptions
were never updated. Now this data lives in the Metadata field where it gets
refreshed on every re-import, while Description holds only the actual DB
table comment.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address PR review feedback:
- Clear known import-generated metadata keys before merging so absent
  metrics (e.g. row_count no longer reported by DB) don't linger
- Extract mergeImportMetadata() helper so test exercises the real code
  path instead of duplicating the merge logic

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@karakanb
karakanb force-pushed the sabrikaragonen/import-metadata-field branch from b5d2568 to 4c4080b Compare August 26, 2026 14:08
@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fix all with Greploop Fix All in Conductor

Reviews (2): Last reviewed commit: "fix(import): refresh metadata across dat..." | Re-trigger Greptile

Comment thread cmd/import.go
Comment on lines +888 to +890
if src.Owner != "" {
dst.Owner = src.Owner
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stale owner survives re-import

When a later database import reports an empty owner, mergeImportMetadata leaves the existing non-empty owner unchanged, causing the outdated owner to remain in the persisted asset YAML.

Suggested change
if src.Owner != "" {
dst.Owner = src.Owner
}
dst.Owner = src.Owner

Fix in Conductor

@karakanb
karakanb merged commit 178b641 into main Aug 27, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants