Major change in stream expected version?

[semion-akimtsev]

Hi @damianh ,

We have recently tried to upgrade to one of 1.2.x pre-release versions and got some unexpected behavior due to this change here

SQLStreamStore/src/SqlStreamStore/Streams/ExpectedVersion.cs

Line 23 in 13ae8dc

public const int NoStream = -3;

compared to version 1.1.x.
So it looks like there should be a major version change. What versioning strategy is applied to this repo?

[yreynhout]

Are you using 1.2.x's ExpectedVersion with an older client (e.g. 1.1.x)? Or are you shipping the constant as part of another lib which you can't upgrade? What was the unexpected behavior (I suspect the reuse of -1)?

[semion-akimtsev]

Well, as ExpectedVersion is an integer, the code which relies on this public contract made some assumptions about -1. After it was switched to 1.2.0-beta.6 some of the checks started to return different results.

[yreynhout]

I've never really liked these as a constant nor as an integer - it should be opaque to the consumer and nobody should ever base decisions on the underlying value - it's a deficiency in SSS's type system. Alas, that ship has sailed for SSS, and so here we are.

[thefringeninja]

As I mentioned in #332, calling code should not just blindly rely on these magic numbers. Use a value object in your application code and translate before you call sql stream store.

…

On Sun, Nov 3, 2019 at 2:06 PM Yves Reynhout ***@***.***> wrote: I've never really liked these as a constant nor as an integer - it should be opaque to the consumer and nobody should ever base decisions on the underlying value - it's a deficiency in SSS's type system. Alas, that ship has sailed for SSS, and so here we are. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#331?email_source=notifications&email_token=AADY7BZ5MEDZNJLMKYFAJE3QR3EBVA5CNFSM4JH4SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5SEFQ#issuecomment-549134870>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADY7BYXK6YIYYHJCAFVZH3QR3EBVANCNFSM4JH4SB2Q> .

-- Sent from my regular computer http://twitter.com/thefringeninja http://www.thefringeninja.com/

[semion-akimtsev]

For now it looks to me that we cannot move on with this version as this is a breaking change, because a public API has been changed in a non-backwards compatible way. And it is not captured in this library's version if I would assume that it uses semantic versioning.

Use a value object in your application code and translate
before you call sql stream store

Could you please give an example?
IMO, since the API exposes a constant, I cannot imagine a way of wrapping it with anything which would not use an if() function of some sort (or directly expose the same value with a different name).

Aside of that, the ExpectedVersion type/contract mixes unrelated concerns together by introducing EmptyStream and NoStream. These have no relation to version. So I would argue that there is a need to introduce an entity called Stream which would have a set of properties exposed like bool=IsEmpty. NoStream does not fit into both approaches as it is neither a version nor a stream status, but probably is a part of a stream "accessor" interface.

[thefringeninja]

It would help a lot if you had example code of what it is you are trying to do.

…

On Mon, Nov 4, 2019 at 9:52 AM Semion Akimtsev ***@***.***> wrote: For now it looks to me that we cannot move on with this version as this is a breaking change, because a public API has been changed in a non-backwards compatible way. And it is not captured in this library's version if I would assume that it uses semantic versioning. Use a value object in your application code and translate before you call sql stream store Could you please give an example? IMO, since the API exposes a constant, I cannot imagine a way of wrapping it with anything which would not use an if() function of some sort (or directly expose the same value with a different name). Aside of that, the ExpectedVersion type/contract mixes unrelated concerns together by introducing EmptyStream and NoStream. These have no relation to version but to the presence status of a stream. So I would argue that there is a need to introduce an entity called Stream which would have a set of properties exposed like bool=IsEmpty. NoStream does not fit into both approaches as it is neither a version nor a stream status, but probably is a part of a stream "accessor" interface. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#331?email_source=notifications&email_token=AADY7B45QZLNEIBODCBF4LDQR7O75A5CNFSM4JH4SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6RWHY#issuecomment-549264159>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADY7B3J7T5MIS3HMFCV7CLQR7O75ANCNFSM4JH4SB2Q> .

-- Sent from my regular computer http://twitter.com/thefringeninja http://www.thefringeninja.com/

[yreynhout]

I agree this is a breaking change in that an existing value was (re)used for a different purpose and constants being copied and embedded into consumers can make this painful. You can only move to this version if you are willing to recompile, yes.