Invalidate model cache when model is registered/updated

dojoengine / dojo

Dojo is a toolchain for building provable games and autonomous worlds with Cairo

https://dojoengine.org

Apache License 2.0

410 stars 169 forks source link

Invalidate model cache when model is registered/updated #1202

Open broody opened 11 months ago

broody commented 11 months ago

Torii stores model schemas in cache so it doesn't have to rebuild from database on every query. When a model is registered/updated, we should invalidate the corresponding cache entry.

Cache implementation: https://github.com/dojoengine/dojo/blob/b3f49977fe921564725c30b912126aea57859bbd/crates/torii/core/src/cache.rs#L17 Subscription event: https://github.com/dojoengine/dojo/blob/b3f49977fe921564725c30b912126aea57859bbd/crates/torii/core/src/sql.rs#L109

CouldBeFree commented 11 months ago

As far as I understand SimpleBroker::publish(model_registered); should call clear method from the ModelCache struct?

broody commented 11 months ago

As far as I understand SimpleBroker::publish(model_registered); should call clear method from the ModelCache struct?

So SimpleBroker will publish an event when a model is registered, we could subscribe to these events in ModelCache and just call update_schema to invalidate that cache entry. Lemme know if you wanna take a stab at it.

CouldBeFree commented 11 months ago

ModelCache should have a subscribe method which should call update_schema?

tarrencev commented 11 months ago

ModelCache should have a subscribe method which should call update_schema?

yes we should create a channel between the register_model processor and the cache. register_model should push a message onto the channel when it is triggered.

more generally, it might be useful to have a generalized pub sub system for processors to emit messages on using channels. we can pass it into each processor similar to the db. that way, processors can notify other parts of the code when they are triggered. we already do this adhoc for set_record and block notifications

CouldBeFree commented 11 months ago

I used publish-subscribe in js in this way.

class PubSub {
    constructor() {
        this.instance = null;
    }

    init() {
        const events = {};

        return {
            subscribe(evName, fn) {
                events[evName] = events[evName] || [];
                events[evName].push(fn);
            },
            unsubscribe(evName, fn) {
                if (events[evName]) {
                    events[evName] = events[evName].filter(f => f !== fn);
                }
            },
            publish(evName, data) {
                if (events[evName]) {
                    events[evName].forEach(f => {
                        f(data);
                    });
                }
            }
        };
    }

    getInstance() {
        if (!this.instance) {
            this.instance = this.init();
        }
        return this.instance;
    }
}

export default PubSub;

And it works in the following way. Some method subscribes to the some event name -> inst.subscribe('ev_name', someDataHandler); In another part of the application, I publish some data under the registered event name -> inst.publish('ev_name', someData); Do we need to implement pub sub in this way?

loothero commented 10 months ago

@broody I am trying to capture this issue with a test case but the current system doesn't appear to support updating an existing model via sql.rs -> register_model

I have tested three variants of updating a model:

Overwrite an existing model with an empty set of children Result: The model is unchanged in the db.
Update the type of an existing child of a model: Result: The child type is not updated in the db
Add a new child to an existing model: Result: database error "(code: 1) no such column: external_new_children"

This leads me to conclude that the current system does not provide the ability to update an existing model. Curious if we have any test cases or real world examples of an existing model being updated. If not and you think the above observation could be valid, I can create an issue for this and push my failing test cases.

broody commented 10 months ago

Hi @loothero!

Mmm u r right, I reproduced issue. Yes, please file an issue. I don't think we have test coverage for this from indexer side.