ava-innersource / Liquid-Application-Framework-1.0-deprecated

Liquid is a framework to speed up the development of microservices
MIT License
25 stars 13 forks source link

SetMediaStorage approach causes unexpected behavior. #73

Open bruno-brant opened 4 years ago

bruno-brant commented 4 years ago

Update:

After taking a deep dive into AzureBlob, I've realized that there's a much worse issue at play here. SetMediaStorage is called by AzureBlob to set itself as the media storage for CosmosDb:

https://github.com/Avanade/Liquid-Application-Framework/blob/ac834f46809cd445dda8783ff616ea4ae7af6b33/src/Liquid.OnAzure/Storages/AzureBlob.cs#L62-L65

The caveat is the if instruction. This means that if the Repository is still null, for some reason, then AzureBlob isn't set as the media storage. However, if later on the repository is set, it will not detect that a MediaStorage was set and will use it's default behavior.

This means that the order of initialization may affect this code - calling Workbench.Use... for MediaStorage before calling it for Repository will lead to this unexpected behavior.

This has to be escalated to a bug, since this may bite our users in the a**.

Old issue:

For good or evil, Liquid is based on a service locator pattern (see #39). There's, however, a method on a base class, ILightRepository, that sets a dependant service to the repository implementation:

https://github.com/Avanade/Liquid-Application-Framework/blob/1f805007f58b73ae09067bc56f863ae0fa702224/src/Liquid.Base/Interfaces/Repository/ILightRepository.cs#L18

My guess is that this is a mistake. Repositories should be using WorkBench to obtain a copy of the ILightMediaStorage to use.

bruno-brant commented 4 years ago

After the update above:

1) There's no escape from a "breaking" change since the current behavior is broken. For now, I recommend making sure that CosmosDb always reads the current MediaStorage from Workbench.

2) The change above impacts CosmosDb, which doesn't yet have unit tests. It'll be necessary to complete #38 before this one.