refactor: persistence table structure

[zepatrik]

Related issue

closes #613
closes #448
closes #665

Proposed changes

As outlined in #613 we now have one table for all relation tuples:

CREATE TABLE keto_relation_tuples
(
    shard_id                 UUID        NOT NULL,
    network_id               UUID        NOT NULL,
    namespace_id             INTEGER     NOT NULL,
    object                   VARCHAR(64) NOT NULL,
    relation                 VARCHAR(64) NOT NULL,
    subject_id               VARCHAR(64) NULL,
    subject_set_namespace_id INTEGER NULL,
    subject_set_object       VARCHAR(64) NULL,
    subject_set_relation     VARCHAR(64) NULL,
    commit_time              TIMESTAMP   NOT NULL,

    PRIMARY KEY (shard_id, network_id),

    -- enforce to have exactly one of subject_id or subject_set
    CONSTRAINT chk_keto_rt_subject_type CHECK
        ((subject_id IS NULL AND
          subject_set_namespace_id IS NOT NULL AND subject_set_object IS NOT NULL AND subject_set_relation IS NOT NULL)
            OR
         (subject_id IS NOT NULL AND
          subject_set_namespace_id IS NULL AND subject_set_object IS NULL AND subject_set_relation IS NULL))
);

Checklist

• I have read the contributing guidelines.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further comments

Outstanding are still
#628
#618
but those will be separate PRs.

[aeneasr]

So how would a migration from the old model to the new model look like? Given that we need that in prod and stage

[aeneasr]

I found a few things:

• Files 20201110175414_relationtuple do not follow the new strategy. I think appending trailing 0 should be enough.
• Missing index for DELETE FROM keto_relation_tuples WHERE nid = ? AND namespace_id = ? AND object = ? AND relation = ? AND subject_id = ?
• Missing index for DELETE FROM keto_relation_tuples WHERE nid = ? AND namespace_id = ? AND object = ? AND relation = ? AND subject_set_namespace_id = ? AND subject_set_object = ? AND subject_set_relation = ?
• Missing index for GetRelationTuples()

And a few more questions:

• What is the difference between RelationQuery and InternalRelationTuple?

Also we do not have network isolation tests yet. Do you want to include that in this PR or in another one?

[zepatrik]

Files 20201110175414_relationtuple do not follow the new strategy. I think appending trailing 0 should be enough.

I can't change the migration ID for already applied migrations right? That would break existing installations as far as I can tell.

---

Missing index for GetRelationTuples()

Absolutely correct, but I would have to index everything in various orders to match all cases. Depending on the application needs you have different queries. That was the whole point of why we had tables per namespace until now, so that we could add indexes matching the application needs, and not having to index everything. IMO we should right now have some slow queries whenever the indexes are not there, but add the ones required for check evaluation. Later we can add configuration to the namespaces that would enable partial indexes, so that we can say in the groups namespace, we want to query by object and relation (think: listing all members), while in the files namespace we want to query by subject and relation (think: listing all files a user is the owner of).

TL;DR

I will add indexes required for the check and expand APIs, but not for user-defined queries.

---

What is the difference between RelationQuery and InternalRelationTuple?

The swagger annotations and receiver functions.

[aeneasr]

I can't change the migration ID for already applied migrations right? That would break existing installations as far as I can tell.

Ah, I missed that the files were moved, not created.

I will add indexes required for the check and expand APIs, but not for user-defined queries.

SGTM

[zepatrik]

@aeneasr this is where we call the isolation test
We have the same registry and therefore same DB connection, but initialize the persisters with different network IDs.

[zepatrik]

This file contains everything relevant for the mapping between the serial namespace ID and the UUID stored in the database.
Because it is expected to have few namespaces that are used all the time, I added a ristretto cache for that mapping.

[zepatrik]

Wow this diff is super messy... Please especially look into this function here, and whether it is a good approach. IMO this is a valid use-case for reflect, but we should think about that.

[aeneasr]

So generally this looks pretty smart. Personally I feel that reflection should not belong into strict enforcement code of things that do isolation. Since we have panics here, it kind of feels ok though!