Skip to content

Conversation

@benjamonnguyen
Copy link

@benjamonnguyen benjamonnguyen commented Apr 27, 2025

Description

  • Implement repository and service level changes to enable updating tag value with logic to propagate update to all descendants.

  • Add option to update tag name on web.

  • Add tags_closure logic to TagRepository.create() and add data fix migration script to cover previous gap.

  • Disallow slashes in tag names in tag create and update calls

How Has This Been Tested?

  • Edit tag name on web, verify db update for target tag and all descendants
image image image image
  • Create tag via "POST /tags" endpoint and verify that tags_closure entry is created
    BadRequest
image

OK
image
image
image
image
image

  • Test migration script

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@benjamonnguyen benjamonnguyen changed the title Allow tag name change on web Enable tag name change on web Apr 27, 2025
@bo0tzz bo0tzz changed the title Enable tag name change on web feat(web): tag renaming Apr 27, 2025
@benjamonnguyen benjamonnguyen force-pushed the main branch 5 times, most recently from ab35dcf to 71b4444 Compare April 29, 2025 08:26
…tag closures to efficiently query for updated tag descendants
@benjamonnguyen benjamonnguyen marked this pull request as ready for review April 29, 2025 08:39
@alextran1502
Copy link
Member

Hello, can you help fix the failed checks?

You will need to run make sql and make open-api for OpenAPI Clients and TYPRORM checks

@alextran1502 alextran1502 requested a review from jrasm91 May 5, 2025 03:06
Copy link
Member

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This looks quite complicated and I'm not sure this is the direction we want to go. I think the algorithm for keeping the closure table and the tag table in sync may be incorrect. I'm on vacation now so will have to revisit this later unless someone else picks it up.


export async function up(db: Kysely<any>): Promise<void> {
const tags = await db.selectFrom('tags').select(columns.tag).execute();
return db.transaction().execute(async (tx) => {
Copy link
Member

Choose a reason for hiding this comment

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

We write migrations with raw SQL not the query builder. Please take a look at other migrations. This is more portable in our opinion.

this.logger.setContext(TagRepository.name);
}

// #region tags
Copy link
Member

Choose a reason for hiding this comment

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

Remove this comment

// #region tags
@GenerateSql({ params: [DummyValue.UUID] })
get(id: string) {
getOne(id: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Rename this back to get

async create(tag: Insertable<Tags>) {
let createdTag: Selectable<Tags>;
await this.db.transaction().execute(async (tx) => {
createdTag = await tx.insertInto('tags').values(tag).returningAll().executeTakeFirstOrThrow();
Copy link
Member

Choose a reason for hiding this comment

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

You can just return the value and assign it to the return of the transaction

return this.db.selectFrom('tags').select(columns.tag).where('id', '=', id).executeTakeFirst();
}

@GenerateSql({ params: [DummyValue.UUID] })
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong as the first param is a list

@danieldietzler
Copy link
Member

danieldietzler commented Jul 10, 2025

Hey @benjamonnguyen would you be able to address jrasm's comments and also rebase this PR on main?

@rammusry

This comment has been minimized.

@alextran1502
Copy link
Member

Close as stale, feel free to reopen when you decide to pick it up

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.

6 participants