refactor: add __eq__ on Table

[aarcex3]

Summary

Implemented the __eq__ method on the Table class to address the issue explained in #1075

Closes

#1075

PD: I would like to check the implementation first, that's why this is a DRAFT. Also I couldn't setup the project for testing, what I mean by that is, I tried using docker to run postgres and cockroach, but no luck. Am I missing something?

Also in which file in tests/table would it be best to add tests for this implementation?

Thanks

[sinisaos]

@aarcex3 Thank you for your effort, but I think your solution is not correct. Sorry if I'm wrong. If you have this example

from piccolo.apps.user.tables import BaseUser
from piccolo_conf import DB

class Project(Table, db=DB):
    name = Varchar(length=100)
    owner = ForeignKey(BaseUser)

project = Project.objects(Project.owner).get(Project._meta.primary_key == 1).run_sync()
owner = BaseUser.objects().get(BaseUser._meta.primary_key == 1).run_sync()

print(project.owner == owner)
#  result is "piccolo_user"."id" = "piccolo_user"."id" and should be True

The solution from issue is better and the result is correct, but mypy complains. I think the solution should be something like this using getattr (Both results are correct and mypy is happy)

def __eq__(self, other) -> bool:
    return isinstance(other, Table) and getattr(
        self, self._meta.primary_key._meta.name, None
    ) == getattr(other, other._meta.primary_key._meta.name, None)

PD: I would like to check the implementation first, that's why this is a DRAFT. Also I couldn't setup the project for testing, what I mean by that is, I tried using docker to run postgres and cockroach, but no luck. Am I missing something?

I have Postgres and Cockroach installed on my local machine (for testing). I don't use Docker for testing so not much help from me. If you need help installing and using Cockroach locally, just ask I'll be glad to help.

Also in which file in tests/table would it be best to add tests for this implementation?

I would write the test here because it makes the most sense to me. Sorry for long comment.

[aarcex3]

Hi @sinisaos, thank you so much for the feedback!!. I will give it a try and let you know.

[dantownsend]

Yeah, the solution that @sinisaos looks right.

When testing, I just run Postgres locally (i.e. not in a container). I use Postgres.app on the Mac, or just plain apt install postgresql on Ubuntu.

[sinisaos]

@aarcex3 For CockroachDB you can also install locally (use the installation for your OS). After installation (this is for a Linux machine, it should be similar on a Mac, I don't know about Windows)

1. Open a terminal and run CockroachDB (in single insecure node)

cockroach start-single-node --insecure

2. Open another terminal and use this command to create the database

cockroach sql --insecure --execute="DROP DATABASE IF EXISTS piccolo CASCADE;CREATE DATABASE piccolo;"

3. And finally you can start testing from piccolo directory

./scripts/test-cockroach.sh

[sinisaos]

@aarcex3 Were you able to run the test on your local machine? Here is the branch with code changes and tests. All the code is formatted and ready to merge (if it's good enough Daniel will merge it) and feel free to use it for this PR. To make sure the formatting and linter are satisfied, just run ./scripts/format.sh and ./scripts/lint.sh from piccolo directory. Thanks again for your interest in Piccolo. Hope this helps.

[aarcex3]

Hey @sinisaos, thank you so much for your help and guidance!!!. You didn't have to do all the work 😅 (I wanted to learn and try). But reading others people code is also useful 🤓. Also, yes, I managed to run cockroach on my machine 🤩.

[sinisaos]

You didn't have to do all the work 😅 (I wanted to learn and try)

@aarcex3 Sorry, you're right. I just wanted to help, because I know when you're contributing to a project for the first time it can be frustrating. I won't make the same mistake again.

I ran the CI workflow and everything passed. Now you have to wait for @dantownsend to review or merge. Cheers

[dantownsend]

The only problem with this at the moment is how it interacts with unsaved objects.

If you create an object, but haven't saved it in the database yet:

band_1 = Band()
band_2 = Band()

If we compare two objects which aren't in the database yet:

>>> band_1 == band_2
???

Or if we compare two objects, one which is in the database, and the other that isn't:

band_3 = await Band.objects().first()
>>> band_1 == band_3
???

If the table uses a Serial column as the primary key, then it doesn't have a proper primary key value until it's saved.

We have an attribute on the table called _exists_in_db, which tells us that the object has been saved to the database. We could use that - so we only compare primary key values if we know the object has been saved.

Or maybe we don't care if it's in the database or not, so this works:

band_4 = Band(id=band_3.id)

>>> band_3 == band_4
True

For Serial columns, the value is initially a QueryString until saved. That create a bit of a gotcha at the moment.

>>> band_1 == band_2
QueryString

So we need to handle that edge case.

It might be a bit of a confusing explanation ... sorry about that. We just need to figure out what makes most sense.

[Skelmis]

Another alternative is if the exists in db isnt set, we could just default to the default super().__eq__ perhaps?

I know other methods such as await Band(id=...).refresh() break as a result of the exists in db flag not being set so there is also precedent for the == to simply error. Also, if people do want to compare things not in db they can just compare the relevant column directly

N.B. code might be minorly wrong, writing on a phone.

[dantownsend]

Yeah, falling back to super().__eq__ sounds like a good approach.

I also realised that _exists_in_db should be set to False in the remove method, but it currently isn't. So have to make sure there are no more gotchas related to _exists_in_db.

[dantownsend]

I didn't add any _exists_in_db comparison in the end. I added some logic to handle the edge case of unsaved Table instances with a Serial primary key.

I made it so you can compare a Table to another Table, and also a Table to the raw primary key value:

band_1 = await Band.objects().where(Band.name == "Pythonistas")

>>> band_1 == band_1
True

>>> band_1 == band_1.id
True

>>> band_1 == 1
True

[dantownsend]

I think this just needs docs now.

[dantownsend]

I've added docs.

I removed comparison with raw primary key values e.g. band == 1 because of some weird edge cases which I thought were confusing.

>>> band.id
1

>>> band.id == 1
True

# This would work, which I found odd:
>>> band.id == True
True

[Skelmis]

Great, thank you for this @aarcex3 :)