Provide more flexibility on how EventStream specs are stored

[peterbroadhurst]

I've hit some problems in having enough flexibility on the Event Streams package, and this PR proposes to solve these:

1. The config sub-section on the input/output object is odd
   • You configure websockets/webhooks with a sub-structure named the same name at the top level, why if you're doing something else do you have to nest it underneath a random config JSON
2. Removing access to the type field - I have some cases where ES are exposed in multiple API endpoints, with different restrictions on the type. Specifically one case where the configuration for using event streams internally, vs. externally.
3. Removing advertising webhooks/websockets fields, if those types are not actually used
4. Customizing the persistence implementation, rather coupled to 1-3 above.

Overall this approach makes the package more flexible, and while it requires a small change to consumers (per the example one), I believe all previous cases can be solved without DB migration.

Also fixes a significant issue, where the foreground context of a HTTP call is used to init a stream on UpsertStream.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ba21054) 99.93% compared to head (00303f7) 99.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
+ Coverage   99.93%   99.98%   +0.04%     
==========================================
  Files          78       78              
  Lines        6416     6437      +21     
==========================================
+ Hits         6412     6436      +24     
+ Misses          3        1       -2     
+ Partials        1        0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[peterbroadhurst]

This is probably the most meaningful starting point for understanding the change.

Before: The had an EventStreamSpec[CT any] that was a struct and you must use it, and put any special config you have under a config subsection.

After: This GenericEventStream is a struct that you can use if you don't have any special config, or you can copy+extend it to add in the config you need.

This was the best compromise I could find that gave the flexibility, without making massive changes.

[peterbroadhurst]

This too has gone from being a mandated implementation, to a reference for you to extend.

[peterbroadhurst]

... which matches up with the fact that the DB migrations are described as reference already, so overall I feel it's much more consistent this way

[peterbroadhurst]

You are no longer constrained to this approach. If you want first-class indexed columns for your extra config, you can do it. However, you have to take ownership of the whole CRUD implementation in that case (rather than using NewGenericEventStreamPersistence).

[peterbroadhurst]

This was an example of a field that needed to be top-level for one implementation for DB indexing, and with the previous structure that meant pushing it down to this common layer.

Now we've addressed that form of extensibility properly, it can go.

[awrichar]

is this still filterable?

[peterbroadhurst]

Good spot, thanks. This should be removed.
An implementation that has extra fields, has to create their own filter list.

[peterbroadhurst]

Also adding some comments to the code to clear up the role of GenericEventStream and these base filters.

[peterbroadhurst]

Ok - commit added now with that

[awrichar]

I'm trying to wrap my head around the migration impact to anything using this before the change. What was stored in "config" vs "webhook_config" and "websocket_config"? Will migration involve extending this generic implementation to continue storing data that was in the removed columns "identity" and "config"?

[peterbroadhurst]

Yes - exactly.
You can still choose to store that configuration in DB fields called identity and config, but you're no longer forced to.

If you were doing so before, you would indeed need to move to using your own Persistence implementation - in fact this library is very opinionated now that if you do want to extend you should take on the persistence implementation yourself.

[peterbroadhurst]

I understand it's quite a big change to the interface, where the migration impact is on consumers who have extended it.

The arguments really for the change are:

1. It's a rather new feature in the library, and I don't have evidence it's used widely yet, so it's a good time to address fundamental issues.
2. As someone trying to use the extensibility before, I found it was extremely difficult to extend previously. Now it's much easier. For me the code migration on the consuming side (about a 1/2 day) was well, well worth it in terms of where it left me for future extensibility.

[awrichar]

OK, makes sense - and the comments added in the code are very helpful to that effect. I agree we're trying to feel out the best way to provide an extensible implementation, and it's probably best to make these changes now.