search

SQLStreamStore / SQLStreamStore

Stream Store library targeting RDBMS based implementations for .NET
MIT License
466 stars 125 forks source link

EmptyStream value inconsistency vs EventStore #332

Open bartelink opened 5 years ago

bartelink commented 5 years ago

In the course of https://github.com/jet/equinox/pull/168, I tripped up during some direct porting of a library that wraps EventStore. The item in question is the naming and values of the Version special values (this applies in 1.2.0-beta4/6)

  1. ES has deprecated EmptyStream in favour of NoStream https://github.com/EventStore/EventStore/blob/03435307176cc7118a0d76657df9f87a47eb8c00/src/EventStore.ClientAPI/ExpectedVersion.cs#L27
  2. SSS still has both https://github.com/SQLStreamStore/SQLStreamStore/blob/6f688d42e34976bf1b047eed396125d02b750143/src/SqlStreamStore/Streams/ExpectedVersion.cs#L12
  3. BUT the SSS NoStream is -3 and the ES one is -1 This happens to cause me a problem as I need the value to be -1 for some formulae to not be off by 2...

I've special cased things, but ideally the names or values would either converge or have a name that makes you think ?

related #331

bartelink commented 5 years ago

Then other inconsistency to note is that ES lib versions >=4 use a Long for the index; perhaps this may offer a way to make any change evident?

thefringeninja commented 5 years ago

I would use a value object in your code to wrap this value. These are magic numbers and probably do not belong on any public api.

On Sat, Nov 2, 2019 at 11:04 PM Ruben Bartelink notifications@github.com wrote:

In the course of jet/equinox#168 https://github.com/jet/equinox/pull/168, I tripped up during some direct porting of a library that wraps EventStore. The item in question is the naming and values of the Version special values (this applies in 1.2.0-beta4/6)

  1. ES has deprecated EmptyStream in favour of NoStream https://github.com/EventStore/EventStore/blob/03435307176cc7118a0d76657df9f87a47eb8c00/src/EventStore.ClientAPI/ExpectedVersion.cs#L27
  2. SSS still has both https://github.com/SQLStreamStore/SQLStreamStore/blob/6f688d42e34976bf1b047eed396125d02b750143/src/SqlStreamStore/Streams/ExpectedVersion.cs#L12
  3. BUT the SSS NoStream is -3 and the ES one is -1 This happens to cause me a problem as I need the value to be -1 for some formulae to not be off by 2...

I've special cased things, but ideally the names or values would either converge or have a name that makes you think ?

related #331 https://github.com/SQLStreamStore/SQLStreamStore/issues/331

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SQLStreamStore/SQLStreamStore/issues/332?email_source=notifications&email_token=AADY7B2UCVAO6SY5NHTD4JLQRX2NBA5CNFSM4JIIP232YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HWMYYRQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADY7B6I6P2DO5DFA5QAT7LQRX2NBANCNFSM4JIIP23Q .

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

bartelink commented 5 years ago

TL;DR my problem is that the API is surprising and one can easily make a mistake when porting from ES to SSS. Yes, I know most go the other direction, but that really isn't the point. I can't think of a way to express "you're not making any sense" without pretty much regurgitating the above

Your answer still leaves my concerns as described above unsolved:

An example of something that would solve the problem is to deprecate EmptyStream and NoStream in SSS and replace with something new which removes the two problems, viz:

  1. the SSS value of NoStream is different to the ES one
  2. the maths doesnt work if NoStream <> -1
thefringeninja commented 5 years ago

Something we are doing in eventstore for v6 is to introduce a value object to represent "AnyStreamRevision":

https://github.com/thefringeninja/EventStore/blob/grpc/src/EventStore.Grpc.Common/AnyStreamRevision.cs

The magic numbers are still there, however they will not be represented that way on the wire:

https://github.com/thefringeninja/EventStore/blob/grpc/src/Protos/Grpc/streams.proto#L109

On Mon, Nov 4, 2019 at 10:33 AM Ruben Bartelink notifications@github.com wrote:

TL;DR my problem is that the API is surprising and one can easily make a mistake when porting from ES to SSS. Yes, I know most go the other direction, but that really isn't the point. I can't think of a way to express "you're not making any sense" without pretty much regurgitating the above

Your answer still leaves my concerns as described above unsolved:

  • (probably too late to change but) avoidable differences in value of the same constant between SSS and ES which mean more special casing vs the ES API because math: [the ES value] of -1 actually makes sense if we are to represent an ExpectedVersion as a Version Number - i.e. when we have 0 events, the version is -1, when we have 2, the value is 1
  • (this is fixable) the one that ES and SSS have in common is deprecated in ES
  • the types between ES and SSS are Int32 vs Int64 (suggesting SSS should go int64 for simiplicity)

An example of something that would solve the problem is to deprecate EmptyStream and NoStream in SSS and replace with something new which removes the two problems, viz:

  1. the SSS value of NoStream is different to the ES one
  2. the maths doesnt work if NoStream <> -1

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SQLStreamStore/SQLStreamStore/issues/332?email_source=notifications&email_token=AADY7BYBT6XUTN4MTDINF5TQR7T43A5CNFSM4JIIP232YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6UVOA#issuecomment-549276344, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADY7B67MZMFK4VLEFYID5LQR7T43ANCNFSM4JIIP23Q .

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

bartelink commented 5 years ago

Right - I agree that's a much clearer way to model it; thanks for taking the time to make it concrete for me

If SSS v2 was to present type ExpectedVersion = Revision of int64 | NoStream | Any | StreamExists, then I'd be forced to pick one. However AIUI, if I want to say "The event I'm writing better be event number 0", I still need to supply an ExpectedVersion value which is Revision -1

Right now, SSS requires an Int32, and the correct value for "I am expecting that I am writing the first event" is -1.

IME people like representing ExpectedVersion as an integer and it works well for a heck of a lot of cases. If we didnt have the legacy of ES having EmptyStream and NoStream being -1 and aligning with the general Revision number value, then there is no problem, but I feel too much logic in the field leans on this.

My problem/the inconsistency I'm calling out:

a) the current ES way of doing that:

b) the SSS ways of doing that:

My bottom line is that 1) I'm only interested in "I am writing event 0and I am writing another event 2) I need a constant for the first case 3) the correct constant has a different name in SSS (use NoStream as EmptyStream is deprecated) vs ES (use NoStream) 4) the common constant (NoStream) has a surprising and different value in SSS (-3`) which a) broke my port b) will require me to put comments in the impl now I've discovered that as there is no reliable way in which I can use a constant and have the code read the same way in SSS and ES

To cut a long story short: if some future SSS v2 design can force people away from this potential bug and align with an ES v6 API, that'd be great.