Handle Routing Incorrectly Returning DataStore rather than Error

[anthony-murphy]

Trying to resolve a handle to a dds from a key on a map which is expected to fail, but instead returns the handle to the data store. Below is the test

        it.only("Failure to Load in Shared String", async ()=>{
            const deltaConnectionServer = LocalDeltaConnectionServer.create();

            const fluidExport: SupportedExportInterfaces = {
                IFluidDataStoreFactory: new TestFluidObjectFactory([[stringId, SharedString.getFactory()]]),
            };
            const text = "hello world";
            const documentId = "sstest";
            { // creating client
                const urlResolver = new LocalResolver();
                const documentServiceFactory = new LocalDocumentServiceFactory(deltaConnectionServer);
                const codeDetails = { package: "no-dynamic-pkg" };
                const codeLoader = new LocalCodeLoader([
                    [codeDetails, fluidExport],
                ]);

                const loader = new Loader({
                    urlResolver,
                    documentServiceFactory,
                    codeLoader,
                });

                const container = await loader.createDetachedContainer(codeDetails);
                const dataObject = await requestFluidObject<ITestFluidObject>(container, "default");
                const sharedString  = await dataObject.root.get<IFluidHandle<SharedString>>(stringId).get();
                sharedString.insertText(0, text);

                await container.attach(urlResolver.createCreateNewRequest(documentId));
            }
            { // normal load client
                const urlResolver = new LocalResolver();
                const documentServiceFactory = new LocalDocumentServiceFactory(deltaConnectionServer);
                const codeDetails = { package: "no-dynamic-pkg" };
                const codeLoader = new LocalCodeLoader([
                    [codeDetails, fluidExport],
                ]);

                const loader = new Loader({
                    urlResolver,
                    documentServiceFactory,
                    codeLoader,
                });

                const container = await loader.resolve(urlResolver.createCreateNewRequest(documentId));
                const dataObject = await requestFluidObject<ITestFluidObject>(container, "default");
                const sharedString  = await dataObject.root.get<IFluidHandle<SharedString>>(stringId).get();
                assert.strictEqual(sharedString.getText(0), text);
            }
            { // failure load client
                const urlResolver = new LocalResolver();
                const realSf: IDocumentServiceFactory =
                    new LocalDocumentServiceFactory(deltaConnectionServer);
                const documentServiceFactory: IDocumentServiceFactory = {
                    ...realSf,
                    createDocumentService: async (resolvedUrl,logger) => {
                        const realDs = await realSf.createDocumentService(resolvedUrl, logger);
                        const mockDs = Object.create(realDs) as IDocumentService;
                        mockDs.policies = {
                            ... mockDs.policies,
                            caching: LoaderCachingPolicy.NoCaching,
                        };
                        mockDs.connectToStorage = async ()=>{
                            const realStorage = await realDs.connectToStorage();
                            const mockstorage = Object.create(realStorage) as IDocumentStorageService;
                            mockstorage.read = async (id)=>{
                                const blob = await realStorage.read(id);
                                const blobObj = JSON.parse(Buffer.from(blob, "Base64").toString());
                                if (blobObj.chunkStartSegmentIndex !== undefined) {
                                    const error = new NetworkErrorBasic(
                                        "Not Found",
                                        undefined,
                                        false,
                                        404);
                                    throw error;
                                }
                                return blob;
                            };
                            return mockstorage;
                        };
                        return mockDs;
                    },
                };
                const codeDetails = { package: "no-dynamic-pkg" };
                const codeLoader = new LocalCodeLoader([
                    [codeDetails, fluidExport],
                ]);

                const loader = new Loader({
                    urlResolver,
                    documentServiceFactory,
                    codeLoader,
                });

                const container = await loader.resolve(urlResolver.createCreateNewRequest(documentId));
                const dataObject = await requestFluidObject<ITestFluidObject>(container, "default");
                const sharedString  = await dataObject.root.get<IFluidHandle<SharedString>>(stringId).get();
                assert.strictEqual(sharedString.getText(0), text);
            }
        });

[anthony-murphy]

@agarwal-navin and @vladsud

[agarwal-navin]

I can take a look at this. Will have to run the test to see what's happening. Why is handle routing supposed to fail here?

[anthony-murphy]

I found this looking for another issue. That issue is that when the snapshot header request fails for the sequence we are not propagating that, which just lead to an empty sequence. Now I throw from load in the sequence to fail loading. I don't know quite what the right behavior is here, my hunch is that that handle get should also throw, but not sure how that is plumbed.

For the above test, i create a mock for the storage service, and throw when it detects a read on the sequence's header blob (same problem as described above). So when we load the shared string via handle i would expect that failure to bubble up some how

[anthony-murphy]

i traced the code a bit, and the load does fail, but then the request handled mixin @vladsud added kicks in, and ends up returning the data store itself

[vladsud]

I did not read it very carefully, but mixinRequestHandler() is only used when we have external handler, and it just calls it if main request failed with 404. If it converts failure to non-failure, that means the request handler that mixinRequestHandler got does it, i.e. it's in user code.

[anthony-murphy]

i didn't look super closely, as trying to figure out the other issue right now. the test repro's it 100%. the last sharedString var will actually be a ITestFluidObject after the handle is retrieved.

[agarwal-navin]

Let me try to repro this and debug.

[agarwal-navin]

I found the issue -
The test is using TestFluidObject which uses mixinRequestHandler to handle requests. When SharedString load fails, data store runtime calls into the TestFluidObject's request handler which just returns itself:

public async request(request: IRequest): Promise<IResponse> {
        return {
            mimeType: "fluid/object",
            status: 200,
            value: this,
        };
    }

Looks like that's the bug. I am not sure why TestFluidObject is using mixin when it doesn't really want to handle requests. We should remove it or handle it appropriately.
I also see lot of examples using mixinRequestHandler similar to TestFluidObject and it does not seem right. We should fix those too.

[vladsud]

Yes, we have a lot of (mostly sample code, but I saw and fixed similar patterns in shipping code) where objects would not even look at request and return itself. That's pretty much always wrong - objects like that should test for query to be empty.

[anthony-murphy]

When SharedString load fails, data store runtime calls into the TestFluidObject's request handler

I don't really understand how this is even ending up on the request function of the test object, that seems wrong to me, and will likely be unexpected by data store authors.

[anthony-murphy]

I think something like this needs to be done:
main...anthony-murphy:handle-resolution

i think 404 is the wrong error, and is handled at other layers, which is what kicks us down the data object request path. then i think we should throw from the get with the channel resolution failure

[vladsud]

I'd say that's good change, but it's not enough. Objects should not handle all requests that are coming their way. They should be explicitly handling "/" only in case of terminating node in request tree.
500 is better as it will help in other places. For example (even though not ideal), hosts today repeat request with wait = true if they got 404 with wait = false. So 404 is truly means means not found, not some other error.

[anthony-murphy]

I agree that object shouldn't handle all request, but we also should only route a request to objects that should reasonably expect them. Happy to commit that change if we think it's good enough.

[agarwal-navin]

We also need to fix the request handler in TestFluidObject and other examples. Created #4621 to track this.

[anthony-murphy]

[anthony-murphy]

i'm almost thinking that dataStoreRuntime should mirror container runtime and have a resolveHandle method rather than going down the request flow here

[anthony-murphy]

closing this as a duplicate of #3062, as the error case is fixed, and #3062 handles the design issue that can lead to this