Enable use of shortids by default (#24142)

This PR enables use of short ids by default. Along with having a
kill-switch configuration available to turn the feature off if needed).
The PR also fixes the corresponding test. The was implemented with
`assetInvert` to negatively test for special characters and short ids
when the bug was not fixed.

Note:
This PR is a follow up from a sequence of fixes that have went in
before, to ensure cross client and cross layer compat.
Short summary - short ids previously generated characters that changed
their form when any code used `encodeURIComponent()`. That has been
fixed by - 1 updating the shortids generator code to only use encode
safe characters + removal of `encodeURIComponent` where it was being
used.
#21680
#24097
#24032

Fixes
[AB#8568](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8568)

Author: pragya91

packages/runtime/container-runtime/src/channelCollection.ts

@@ -621,11 +621,11 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 	 */
 	protected createDataStoreId(): string {
 		/**
-		 * There is currently a bug where certain data store ids such as "[" are getting converted to ASCII characters
-		 * in the snapshot.
-		 * So, return short ids only if explicitly enabled via feature flags. Else, return uuid();
+		 * Return uuid if short-ids are explicitly disabled via feature flags.
 		 */
-		if (this.mc.config.getBoolean("Fluid.Runtime.IsShortIdEnabled") === true) {
+		if (this.mc.config.getBoolean("Fluid.Runtime.DisableShortIds") === true) {
+			return uuid();
+		} else {
 			// We use three non-overlapping namespaces:
 			// - detached state: even numbers
 			// - attached state: odd numbers
@@ -641,7 +641,6 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 			}
 			return id;
 		}
-		return uuid();
 	}
 
 	public createDetachedDataStore(

packages/runtime/datastore/src/dataStoreRuntime.ts

@@ -459,11 +459,11 @@ export class FluidDataStoreRuntime
 			this.validateChannelId(id);
 		} else {
 			/**
-			 * There is currently a bug where certain data store ids such as "[" are getting converted to ASCII characters
-			 * in the snapshot.
-			 * So, return short ids only if explicitly enabled via feature flags. Else, return uuid();
+			 * Return uuid if short-ids are explicitly disabled via feature flags.
 			 */
-			if (this.mc.config.getBoolean("Fluid.Runtime.IsShortIdEnabled") === true) {
+			if (this.mc.config.getBoolean("Fluid.Runtime.DisableShortIds") === true) {
+				id = uuid();
+			} else {
 				// We use three non-overlapping namespaces:
 				// - detached state: even numbers
 				// - attached state: odd numbers
@@ -480,8 +480,6 @@ export class FluidDataStoreRuntime
 						this.dataStoreContext.containerRuntime.generateDocumentUniqueId?.() ?? uuid();
 					id = typeof res === "number" ? encodeCompactIdToString(2 * res + 1, "_") : res;
 				}
-			} else {
-				id = uuid();
 			}
 			assert(!id.includes("/"), 0x8fc /* slash */);
 		}

packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts

@@ -945,25 +945,13 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 		);
 	});
 
-	/**
-	 * Function that asserts that the value is not as expected. e have a bug in one of our customer's app where a short
-	 * data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in
-	 * certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot
-	 * cannot find the data store.
-	 *
-	 * While we figure out the fix, we are disabling the ability to create short IDs and this assert validates it.
-	 */
-	function assertInvert(value: boolean, message: string) {
-		assert(!value, message);
-	}
-
 	async function TestCompactIds(enableRuntimeIdCompressor: IdCompressorMode) {
 		const container = await createContainer({
 			runtimeOptions: { enableRuntimeIdCompressor },
 		});
 		const defaultDataStore = (await container.getEntryPoint()) as ITestDataObject;
 		// This data store was created in detached container, so it has to be short!
-		assertInvert(
+		assert(
 			defaultDataStore._runtime.id.length <= 2,
 			"short data store ID created in detached container",
 		);
@@ -1002,14 +990,14 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 		// Check directly that ID compressor is issuing short IDs!
 		// If it does not, the rest of the tests would fail - this helps isolate where the bug is.
 		const idTest = defaultDataStore._context.containerRuntime.generateDocumentUniqueId();
-		assertInvert(typeof idTest === "number" && idTest >= 0, "short IDs should be issued");
+		assert(typeof idTest === "number" && idTest >= 0, "short IDs should be issued");
 
 		// create another datastore
 		const ds2 = await defaultDataStore._context.containerRuntime.createDataStore(pkg);
 		const entryPoint2 = (await ds2.entryPoint.get()) as ITestDataObject;
 
 		// This data store was created in attached  container, and should have used ID compressor to assign ID!
-		assertInvert(
+		assert(
 			entryPoint2._runtime.id.length <= 2,
 			"short data store ID created in attached container",
 		);
@@ -1028,7 +1016,7 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 			undefined,
 			SharedDirectory.getFactory().type,
 		);
-		assertInvert(channel.id.length <= 2, "DDS ID created in detached data store");
+		assert(channel.id.length <= 2, "DDS ID created in detached data store");
 
 		// attached data store.
 		await ds2.trySetAlias("foo");
@@ -1043,7 +1031,7 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 			undefined,
 			SharedDirectory.getFactory().type,
 		);
-		assertInvert(channel2.id.length <= 2, "DDS ID created in attached data store");
+		assert(channel2.id.length <= 2, "DDS ID created in attached data store");
 	}
 
 	it("Container uses short DataStore & DDS IDs in delayed mode", async () => {
@@ -1062,19 +1050,17 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 		};
 		const container = await loader.createDetachedContainer(defaultCodeDetails);
 		const defaultDataStore = (await container.getEntryPoint()) as ITestFluidObject;
-		assertInvert(
+		assert(
 			defaultDataStore.context.id.length <= 2,
-			"Default data store's ID should be short",
+			"Default data store's ID should be short.",
 		);
 		const dataStore1 =
 			await defaultDataStore.context.containerRuntime.createDataStore(TestDataObjectType);
 		const ds1 = (await dataStore1.entryPoint.get()) as ITestFluidObject;
-		assertInvert(
+		assert(
 			ds1.context.id.length <= 2,
 			"Data store's ID in detached container should not be short",
 		);
-		const dds1 = SharedDirectory.create(ds1.runtime);
-		assertInvert(dds1.id.length <= 2, "DDS's ID in detached container should not be short");
 
 		await container.attach(provider.driver.createCreateNewRequest());
 
@@ -1085,15 +1071,14 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, com
 			ds2.context.id.length > 8,
 			"Data store's ID in attached container should not be short",
 		);
-		const dds2 = SharedDirectory.create(ds2.runtime);
-		assert(dds2.id.length > 8, "DDS's ID in attached container should not be short");
 	});
 });
 
 /**
- * These tests repro a bug where ODSP driver does not correctly decode encoded snapshot tree paths.
- * Data store / DDS created with special characters are encoded during summary upload but during
- * download, they are not correctly decoded in certain scenarios.
+ * These tests try to reproduce a bug where ODSP driver did not correctly decode encoded snapshot tree paths.
+ * Data store / DDS created with special characters were encoded during summary upload but during
+ * download, they were not correctly decoded in certain scenarios.
+ * The bug has been resolved, but these tests are kept to ensure that the bug does not regress.
  */
 describeCompat(
 	"Short IDs in detached container",
@@ -1119,7 +1104,6 @@ describeCompat(
 			if (provider.driver.type !== "odsp") {
 				this.skip();
 			}
-			configProvider.set("Fluid.Runtime.IsShortIdEnabled", true);
 		});
 
 		/**

packages/test/test-end-to-end-tests/src/test/summarization/summaryHandles.spec.ts

@@ -62,8 +62,6 @@ describeCompat.skip(
 		});
 
 		it("A data store id with special character `[` works properly with summary handles", async () => {
-			// Enable short ids for this test to create a data store with special character.
-			configProvider.set("Fluid.Runtime.IsShortIdEnabled", true);
 			// Enable single-commit summaries so that when the test runs with old odsp driver, it doesn't fail with summary nacks.
 			configProvider.set("Fluid.Container.summarizeProtocolTree2", true);
 			const container = await provider.createDetachedContainer(runtimeFactory, {