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!: EXPOSED-577 Allow Entity and EntityID parameters to not be Comparable #2277

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

bog-walk
Copy link
Member

@bog-walk bog-walk commented Oct 17, 2024

Description

Summary of the change:
All Entity related objects and functions are no longer restricted to type-parameter Comparable.

Detailed description:

  • Why:
    Introduction of Kotlin 2.0 brought kotlin.uuid.Uuid, which does not implement Comparable interface, and many requests to allow its usage with IdTable. Loosening these type restrictions will allow users to easily implement their own UuidColumnType and IdTable<Uuid>, at least until the new type is no longer @ExperimentalUuidApi and built-in Exposed api options are made available.

  • How:

    • Any entity related function type-parameter restriction T : Comparable<T> has been replaced with T : Any.
    • Min and Max class operators are also no longer restricted to Comparable types, so that max() and min() can be called on id. This was done because some tests presented this use case.
    • Reference functions are also no longer restricted to Comparable to allow use of these functions with id columns.
    • EntityID and CompositeID are no longer Comparable, since their wrapped values are no longer restricted to this. This means that any use of these directly with comparison operators, or any variant of sort or compareTo(), will no longer compile:
// will no longer work:
entity1.id >= entity2.id

// must be replaced with:
entity1.id.value >= entity2.id.value
// and will only compile if the values are actually comparable

Type of Change

Please mark the relevant options with an "X":

  • New feature

Updates/remove existing public API methods:

  • Is breaking change

Affected databases:

  • All

Checklist

  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs
  • Documentation for my change is up to date

Related Issues

EXPOSED-577

@bog-walk bog-walk linked an issue Oct 17, 2024 that may be closed by this pull request
class Max<T : Comparable<T>, in S : T?>(
class Max<T : Any, in S : T?>(
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes were only made because our tests had use cases like TestTable.id.max().

@e5l @obabichevjb This now leads to the question, what about other restricted functions, like Avg(), which only accepts Comparable column types?
Should it still be possible to call avg() on an entityID column like TestTable.id.avg() or does that use case make no sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That’s a good question. At first glance, it seems like it shouldn't be Comparable, especially for Max functions, if it’s only comparable on the database side. I can’t think of a specific example right now, but imagine something that can have Max applied to it in the database but doesn’t have a comparable representation in Kotlin.

Since we get from SQL just single value and don't compare it on client side, it could be non-comparable.

get() = _idColumns.ifEmpty {
val message = "Table definition must include id columns. Please use Column.entityId() or IdTable.addIdColumn()."
exposedLogger.error(message)
error(message)
}

/** Adds a column to [idColumns] so that it can be used as a component of the [id] property. */
protected fun <S : Comparable<S>> addIdColumn(newColumn: Column<EntityID<S>>) {
protected fun <S : Any> addIdColumn(newColumn: Column<EntityID<S>>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm, is Any here to indicate that S not nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was the assumption I made about most cases. Do you think it's not an ok assumption that may lead to breaking changes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was non-nullable Comparable, so I expect that it should be fine to replace it with non-nullable Any. To make it weaker in the future should be easier then vice versa anyway.

class Max<T : Comparable<T>, in S : T?>(
class Max<T : Any, in S : T?>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

That’s a good question. At first glance, it seems like it shouldn't be Comparable, especially for Max functions, if it’s only comparable on the database side. I can’t think of a specific example right now, but imagine something that can have Max applied to it in the database but doesn’t have a comparable representation in Kotlin.

Since we get from SQL just single value and don't compare it on client side, it could be non-comparable.

- EntityID still implements Comparable interface itself
…parable

- Add breaking changes notes
- EntityID and CompositeID no longer implement Comparable
- Min and Max are no longer restricted to Comparable
- References are also no longer restricted
…parable

- Add note in breaking change about use of entity id with functions restricted
to Comparable
@bog-walk bog-walk force-pushed the bog-walk/loosen-entity-id-type branch from 3eb80f2 to 5fd1a42 Compare October 21, 2024 20:49
@bog-walk bog-walk merged commit d48ab2d into main Oct 21, 2024
5 checks passed
@bog-walk bog-walk deleted the bog-walk/loosen-entity-id-type branch October 21, 2024 22:15
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.

Can't reference on binary primaryKey
3 participants