Lookup lifecycle reducers by lifecycle flag, not by name

[coolreader18]

Description of Changes

I uplifted the ReducerId lookup stuff from module_host into schema, so that it lets the ModuleHost look up lifecycle reducers by their ids. I was a bit confused about ReducersMap, since it claimed to sort the reducer names but afaict did nothing of the sort. Is it that the RawReducerDefV9s supposed to be sorted? Cause if so I can't find any validation code enforcing it.

Expected complexity level and risk

2

Testing

It's not possible to fully test this since we don't have csharp modules in smoketests yet, but the test_lifecycle_reducers_cant_be_called test still passes and host doesn't mention the dunderscore names at all.

[cloutiertyler]

Considering we were able to add windows to CI, how difficult would it be to add CSharp modules to the smoketests?

[coolreader18]

I think @RReverser was working on that? That might've been the wrong impression though

[RReverser]

how difficult would it be to add CSharp modules to the smoketests?

Windows is not a requirement for that, as we already run unit and integration tests for C# modules in Linux-based Github Actions with no issues. It's just a matter of someone actually writing said tests.

Here we don't even need smoketests actually, integration tests are enough. C# modules were already emitting correct modules and we have a test for connect/disconnect events. The problem is that Rust counterpart wasn't (isn't?) ready yet, so our tests have to match Rust naming, which hid the issue that occurs when using custom reducer names.

Once Rust is fixed to preserve custom reducer names as well (#1891), we'll just need to change the names of reducers in

SpacetimeDB/modules/sdk-test-connect-disconnect-cs/Lib.cs

Lines 19 to 30 in c240426

	// TODO: these method names shouldn't be special, but for now they have to match Rust snapshots.
	// See https://github.com/clockworklabs/SpacetimeDB/issues/1891.
	// For now, disable the error that would be raised due to those names starting with `__`.
	#pragma warning disable STDB0009
	[SpacetimeDB.Reducer(ReducerKind.ClientConnected)]
	public static void __identity_connected__(ReducerContext ctx)
	{
	ctx.Db.connected.Insert(new Connected { identity = ctx.CallerIdentity });
	}
	[SpacetimeDB.Reducer(ReducerKind.ClientDisconnected)]
	public static void __identity_disconnected__(ReducerContext ctx)

, and it will close this testing gap.

[RReverser]

I was a bit confused about ReducersMap, since it claimed to sort the reducer names but afaict did nothing of the sort.

That might be outdated comment. They used to be sort, but they must not be - in fact, that was the cause of an issue fixed by #1987.

[coolreader18]

If I just make that change as part of this, it'll fix the testing issue, and it's really pretty straightforward.

[RReverser]

That does sound great.

Once you fix the Rust bindings to preserve reducer names, the ensure_same_schema test should immediately start failing (as expected), and then the linked C# counterpart can be renamed to original reducer names as well.

[coolreader18]

@RReverser they seem to not be failing?

[bfops]

LGTM on my codeowned files (I didn't review the rest of the PR).

[RReverser]

@RReverser they seem to not be failing?

Hmm. Probably because those reducers are ignored by ponder_auto_migrate because we filter them out now.

SpacetimeDB/crates/schema/tests/ensure_same_schema.rs

Line 18 in c240426

let diff = ponder_auto_migrate(&cs, &rs)

We used to preserve them so they were exposed both as regular reducers and as event handlers, but the filtering logic changed as part of this PR.

We'll need to enhance the diffing test to account for those hidden reducers in the future, but for now can you change connect-disconnect's Lib.cs to match the rust version regardless please?

[kazimuth]

Approved once my caveat is addressed.

(In retrospect I wish we'd just done reducer_id_from_name but it's too late now.)