Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): sidecar in asset_files #16827

Draft
wants to merge 86 commits into
base: main
Choose a base branch
from
Draft

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Mar 12, 2025

  • add new sidecar type in asset_files
  • remove sidecarPath from asset
  • create dedicated assetFileRepository

etnoy added 30 commits December 2, 2024 22:46
etnoy and others added 22 commits February 17, 2025 23:41
@etnoy etnoy force-pushed the feat/sidecar-asset-files branch 2 times, most recently from ae1d2ee to 43eb083 Compare March 12, 2025 12:17
Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

I think there's not only the database table shift here but also a refactor around how sidecar files are handled/discovered in general, right? I think that'd be easier to review as a separate PR.

@@ -18,12 +18,15 @@ export class AssetFileEntity {
id!: string;

@Index('IDX_asset_files_assetId')
@Column()
@Column({ nullable: true, default: null })
Copy link
Member

Choose a reason for hiding this comment

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

Is it a good idea to make this nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sidecars are now discovered independently of files in library scans can are possibly orphans

name = 'Test1741731070431';

public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`ALTER TABLE "assets" DROP COLUMN "sidecarPath"`);
Copy link
Member

Choose a reason for hiding this comment

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

Should this move the existing sidecar paths over to asset_files rather than just dropping them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll think about this later

@GenerateSql({
params: [{ libraryId: DummyValue.UUID, importPaths: [DummyValue.STRING], exclusionPatterns: [DummyValue.STRING] }],
})
async detectOfflineExternalSidecars(
Copy link
Member

Choose a reason for hiding this comment

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

Sidecars are already treated as malleable, so I don't think sidecar handling should need any special cases for external.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a speedup that we can do for external assets

@etnoy etnoy force-pushed the feat/sidecar-asset-files branch from 43eb083 to ec9d538 Compare March 12, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants