Skip to content

feat(snowflake): extend Entity class to support snowflakes #56795

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.

Sign up for GitHub

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

Jump to bottom
Open
miaulalala wants to merge 11 commits into
base: master
Choose a base branch
from
Open

feat(snowflake): extend Entity class to support snowflakes #56795

miaulalala wants to merge 11 commits into from
+296 −186

Conversation

@miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Dec 2, 2025 •
edited
Loading

RFC: extending the Entity class to support snowflake IDs automagically

Good ides - yes / no?

Benefits:

  • every entity that supports snowflake IDs can automagically handle them by implementing the SnowflakeAwareEntity instead of the regular Entity
  • No adding IGenerator / IDecoder in every entity class
  • centralised method support for createdAt, setId, and anything else we would like to offer separately
  • get entire decoded snowflake id from the entity

Drawbacks:

  • Will it work the way I think it should?
  • Resolves: #

TODO

  • ...

Checklist

@miaulalala miaulalala added this to the Nextcloud 33 milestone Dec 2, 2025
@miaulalala miaulalala self-assigned this Dec 2, 2025
@miaulalala miaulalala requested a review from a team as a code owner December 2, 2025 13:10
@miaulalala miaulalala requested review from ArtificialOwl and icewind1991 and removed request for a team December 2, 2025 13:10
provokateurin
provokateurin reviewed Dec 2, 2025
View reviewed changes
CarlSchwan
CarlSchwan reviewed Dec 2, 2025
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Llike the Id :)

nickvergessen
nickvergessen reviewed Dec 2, 2025
View reviewed changes
@come-nc
Copy link
Contributor

come-nc commented Dec 2, 2025

We should rename IGenerator and IDecoder to ISnowflakeGenerator and ISnowflakeDecoder, otherwise after a use all code is ambiguous.
A value object for decoded snowflakes would be good as well.

@miaulalala
Copy link
Contributor Author

miaulalala commented Dec 3, 2025

We should rename IGenerator and IDecoder to ISnowflakeGenerator and ISnowflakeDecoder, otherwise after a use all code is ambiguous. A value object for decoded snowflakes would be good as well.

Added

Altahrim
Altahrim reviewed Dec 4, 2025
View reviewed changes
Altahrim
Altahrim reviewed Dec 4, 2025
View reviewed changes
Copy link
Collaborator

@Altahrim Altahrim left a comment

Choose a reason for hiding this comment

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

If we introduce a Snowflake DTO, I think Decoder should return it directly

@miaulalala
Copy link
Contributor Author

miaulalala commented Dec 4, 2025

Would it make sense to cache the decoded snowflake ID? Seems like there's quite a few expensive calculations going on.

@Altahrim
Copy link
Collaborator

Altahrim commented Dec 4, 2025

We can still add a cache later if needed but decoding on 64 bits mainly involve binary operations, it should be fast.
32 bit is more complicated 😓

@miaulalala miaulalala requested a review from Altahrim December 4, 2025 13:51
Altahrim
Altahrim reviewed Dec 4, 2025
View reviewed changes
nickvergessen
nickvergessen reviewed Dec 5, 2025
View reviewed changes
CarlSchwan
CarlSchwan approved these changes Dec 15, 2025
View reviewed changes
nickvergessen
nickvergessen reviewed Dec 15, 2025
View reviewed changes
nickvergessen
nickvergessen reviewed Dec 15, 2025
View reviewed changes
@miaulalala miaulalala force-pushed the feat/noid/extend-entity-to-be-snoflake-aware branch 2 times, most recently from 83d0781 to 7d1ded5 Compare December 16, 2025 11:14
come-nc
come-nc reviewed Dec 16, 2025
View reviewed changes
come-nc
come-nc approved these changes Dec 16, 2025
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I would also be on the "public readonly properties" team, but that’s bikeshedding.

@miaulalala miaulalala force-pushed the feat/noid/extend-entity-to-be-snoflake-aware branch from 7d1ded5 to 75a21de Compare December 16, 2025 15:14
@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 16, 2025
Altahrim
Altahrim approved these changes Dec 16, 2025
View reviewed changes
Copy link
Collaborator

@Altahrim Altahrim left a comment

Choose a reason for hiding this comment

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

With same comment than Côme :)

provokateurin
provokateurin requested changes Dec 18, 2025
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Some small changes would be nice. Also I would prefer if the changes were done in multiple commits. I also noticed the name of the DI variable is inconsistent, maybe you can fix that as well.

miaulalala added a commit to nextcloud/spreed that referenced this pull request Dec 18, 2025
@miaulalala
refactor: use SnowflakeAwareEntity
7a7c297 
- [ ] needs nextcloud/server#56795
- [ ] needs #16508

Skipping CI for now, amend the commit message and remove the skip-ci param when ready to rebase and merge

[skip-ci]

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala force-pushed the feat/noid/extend-entity-to-be-snoflake-aware branch from 75a21de to 8d35e95 Compare December 18, 2025 22:34
@miaulalala miaulalala mentioned this pull request Dec 18, 2025
Draft
5 tasks
miaulalala added a commit to nextcloud/spreed that referenced this pull request Dec 18, 2025
@miaulalala
refactor: use SnowflakeAwareEntity
da021c2 
- [ ] needs nextcloud/server#56795
- [ ] needs #16508

Skipping CI for now, amend the commit message and remove the skip-ci param when ready to rebase and merge

[skip ci]

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala
refactor!: Rename Snowflake Generator and Decoder
3ae4815 
and introduce the Snowflake DTO

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala
feat: extend Entity and adjust QBMapper to support Snowflake IDs
6ae368c 
Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala
fixup! refactor!: Rename Snowflake Generator and Decoder and introduc…
dcbc671 
…e the Snowflake DTO
@miaulalala
fixup! refactor!: Rename Snowflake Generator and Decoder and introduc…
11459c2 
…e the Snowflake DTO
@miaulalala
refactor: move existing usages of snoflake IDs SnowflakeAwareEntity
36fe8be 
Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala
fixup! feat: extend Entity and adjust QBMapper to support Snowflake IDs
44ffccd
@miaulalala
fixup! refactor: move existing usages of snoflake IDs SnowflakeAwareE…
577b145 
…ntity
@miaulalala
fixup! feat: extend Entity and adjust QBMapper to support Snowflake IDs
9c46c77
@miaulalala
fixup! feat: extend Entity and adjust QBMapper to support Snowflake IDs
d3bc137
@miaulalala
fixup! feat: extend Entity and adjust QBMapper to support Snowflake IDs
47936fa
@miaulalala
fixup! feat: extend Entity and adjust QBMapper to support Snowflake IDs
67b168a
@miaulalala miaulalala force-pushed the feat/noid/extend-entity-to-be-snoflake-aware branch from ba2f797 to 67b168a Compare December 23, 2025 08:49
CarlSchwan
CarlSchwan reviewed Dec 23, 2025
View reviewed changes
lib/public/AppFramework/Db/SnowflakeAwareEntity.php
$this->id = Server::get(ISnowflakeGenerator::class)->nextId();
$this->markFieldUpdated('id');
}
}
Copy link
Member

@CarlSchwan CarlSchwan Dec 23, 2025

Choose a reason for hiding this comment

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

this needs a getId method which returns a string. We can't relly on the one from Entity as it returns an int and this will break on 32 bits system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@provokateurin provokateurin provokateurin requested changes

@Altahrim Altahrim Altahrim approved these changes

@come-nc come-nc come-nc approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Requested changes must be addressed to merge this pull request.

Assignees

@miaulalala miaulalala

Labels

3. to review Waiting for reviews enhancement standardization

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

7 participants

@miaulalala @come-nc @Altahrim @nickvergessen @CarlSchwan @provokateurin