Handle failed cache deletion

[SnowMarble]

Here's the changes for this PR

• failed to delete a cache, save the key in a Set so that we can treat it as deleted.
• handled promise function calls.
• setup test and add unit and integration test
• introduce Serializable
• update mikro orm version to 6

[SnowMarble]

@ramiel Hi, just wanted to send a quick reminder. It'd be great to review the PR when you have a chance. Thanks!

[ramiel]

Sorry, I completely missed it. I'll have a look at it soon.

[ramiel]

Hi @SnowMarble , I just went through the PR and I'm impressed and yet confused. I think we can split your PR into two parts:

• handle failed deletes
• add tests

Honestly, my concerns are about the "handling of deletion" part. I'm wondering what's the use case that let you decide to implement it. My first thought was: if such a deletion error happens, shouldn't it be handled by mikroORM? If any engine (redis, memory,etc.) fails to delete, the problem should be handled uniquely by the upper code, in MikroORM.

Another point is that the failed deletion is handled in memory. That will led to inconsistencies if more instances are running at the same time, which is pretty common in a production scenario.

Don't get me wrong, your code is ok, I just don't think this is the way it should be handled.

I'm happy to keep discussing about this, but I can't merge it. Maybe you want to start from an issue or a real use case you had?

[SnowMarble]

Great point. My second thought, it would be better to throw a proper error, so the developers can decide what to do. Is it okay to revert the change and do this? Then I will split the Serializable and testing part into two PRs.

[ramiel]

Yes, it would be better but let's discuss about other feature.

• Serializable

This library is meant to work as result cache for MikroORM. MikroORM defines the interface for custom adapters and this is one. The interface is the following:

export interface CacheAdapter {

  /**
   * Gets the items under `name` key from the cache.
   */
  get(name: string): Promise<any>;

  /**
   * Sets the item to the cache. `origin` is used for cache invalidation and should reflect the change in data.
   */
  set(name: string, data: any, origin: string, expiration?: number): Promise<void>;

  /**
   * Removes the item from cache.
   */
  remove(name: string): Promise<void>;

  /**
   * Clears all items stored in the cache.
   */
  clear(): Promise<void>;

  /**
   * Called inside `MikroORM.close()` Allows graceful shutdowns (e.g. for redis).
   */
  close?(): Promise<void>;

}

As you can see there's no need to forcefully imply the data must be of any specific type. The Serializable type would enforce data type only at build time, but mikroORM would ignore that at runtime. So, the developers would never see any error for any data they want to save on the cache because they only need to stick with mikroORM types, not this adapter types. Again, maybe I'm missing something here, but can we start from your use cases?

• Tests

Thanks for providing tests. There's no really need for the integration test, because that is testing that mikroORM is working, the other test are ok but the part that test the deletion object.

Again, you did a very good job, sorry for not accepting this PR as whole

[SnowMarble]

Thanks again for your opinion. I'm happy to have a discussion about it, and this is my fault for not having enough discussion before making the PR.

As you can see there's no need to forcefully imply the data must be of any specific type. The Serializable type would enforce data type only at build time, but mikroORM would ignore that at runtime

The reason I decided to add the Serializable type is to make sure taht any value is serializable with the v8 serialize api. Of course, the implements has no impact on the runtime, but developers know if their code will work with this library. And I believe that since an adapter can be implemented in any way, so the interface allows the most generous type; any.

This type is not always needed. You'll need it when you're not sure if some unusual types are okay to use. If a developer doesn't pay attention to the logs, they won't even know if the cache failed just because of non-serializable types.

I'm not trying to solve my specific use cases, but possible. But now I also kind of think that this is too early. Because I don't know how many non-serializable values are used in real projects. If you agree, I'll postpone it until I find a more convincing reason, including integration testing (it was more for testing the serializable type). Sorry for confusing you.

[ramiel]

The reason I decided to add the Serializable type is to make sure taht any value is serializable with the v8 serialize api. Of course, the implements has no impact on the runtime, but developers know if their code will work with this library. And I believe that since an adapter can be implemented in any way, so the interface allows the most generous type; any.

I thought this was your reason and my point is that developers using this adapter will never meet any type error when writing their code. This is because they never interact directly with this library, but only with mikroORM and in mikroORM, as you said, the type for the serializable data is any. Tell me if I'm wrong or if this explanation is not clear enough. Again, maybe I wrong.

If you agree, I'll postpone it until I find a more convincing reason, including integration testing (it was more for testing the serializable type). Sorry for confusing you.

Yes please. I don't want to minimize the good work you've done here but you're right, maybe it's too early.

[SnowMarble]

Awesome. And I totally see your point. I made a new PR for the remove error and tests (#9).