false positive migrations detection dependent on encoding

[ivanbelenky]

Disclaimer: I lack knowledge on redis internals, and on the design criteria of the object mapping library.

Issue:
The migrator when running through the model_registry performs a hash calculation for the current schema

conn = cls.db()
try:
    schema = cls.redisearch_schema()
except NotImplementedError:
    log.info("Skipping migrations for %s", name)
    continue
current_hash = hashlib.sha1(schema.encode("utf-8")).hexdigest()

to then compare it to the one it gets from the db

stored_hash = conn.get(hash_key)
schema_out_of_date = current_hash != stored_hash

a connection without decoded_responses=True (and maybe with different charset) will create false positive migrations, because of distinct types, i.e. bytes and str for stored_hash and current_hash respectively.

I guess that a default encoding schema for bytes responses would suffice, or a user warning.

[slorello89]

Can you provide a reproduction of a case where a model in Redis displays a false positive change?

[ivanbelenky]

Sure thing @slorello89, forgot about that

Having the following model, define a database with undecoded responses

import redis
from redis_om import Field, Migrator, HashModel

class User(HashModel):
    first_name: str
    last_name: str = Field(index=True)

    class Meta:
        database = redis.from_url("redis://0.0.0.0:6379", decode_responses=False)

and then just try to migrate that model

m = Migrator()

# should migrate
assert len(m.migrations) == 0
m.detect_migrations()
assert len(m.migrations) > 0

# run, 
m.run()

# reset migrations because migrator does not 
m.migrations = []
# already migrated, should not have migrations after detection
m.detect_migrations()
assert len(m.migrations) == 0 # raises

(for true positive migration checks, you can easily verify that the above code does not raise, if you change to decode_responses=True, there will be no error. Remember of course to wipe out whatever was written into the db)

[ivanbelenky]

@slorello89 any update with regards to this?