ravendb / ravendb-nodejs-client

RavenDB node.js client
MIT License
63 stars 32 forks source link

GetDatabaseOperation throws when response is "successfully" a 404 #365

Closed kamranayub closed 1 year ago

kamranayub commented 1 year ago

Using this code snippet:

async function createDbIfNotExists(store: IDocumentStore) {
    const getDbOp = new GetDatabaseRecordOperation(store.database);
    const dbRecord: DatabaseRecord = await store.maintenance.send(getDbOp);

    console.log('DatabaseRecord does not exist for: ', store.database);

    if (!dbRecord) {
        const createResult = await store.maintenance.send(
            new CreateDatabaseOperation(
                {
                    databaseName: store.database,
                }
            )
        );

        if (createResult?.name) {
            console.log('Automatically created database that did not exist: ' + createResult.name);
        }
    }
}

The code will fail on await store.maintenance.send(getDbOp) with an InvalidOperationException: Response is invalid error.

The reason is because this code path goes through RavenCommand._executeOnSpecificNode and the code path that handles the DatabaseNotFound is unreachable on a 404 response.

image

image

I was modeling this after the equivalent .NET SDK code where I can check for nullish if the DB does not exist and was hoping to do the same.

As a workaround, I believe I can simply have it try to create the DB anyway and let it fail silently.

I also had to use conventions.disableTopologyUpdates = true; since that was failing on initialize due to the DB missing.


SDK Version: 5.2.8 Platform: Windows Node.js: 16.8.1

ml054 commented 1 year ago

@kamranayub

Could you tell me how you init document store?

I wrote following test:

it("dbrecord", async ()=> {
        const store = new DocumentStore("http://live-test.ravendb.net/", "marcin");
        store.initialize();

        const getDbOp = new GetDatabaseRecordOperation(store.database);
        const dbRecord = await store.maintenance.send(getDbOp);

        console.log('DatabaseRecord does not exist for: ', store.database);

        if (!dbRecord) {
            const createResult = await store.maintenance.send(
                new CreateDatabaseOperation(
                    {
                        databaseName: store.database,
                    }
                )
            );

            if (createResult?.name) {
                console.log('Automatically created database that did not exist: ' + createResult.name);
            }
        }
    })

And it properly throws with:

DatabaseDoesNotExistException: marcin
    at getError (src\Exceptions\index.ts:40:19)
    at throwError (src\Exceptions\index.ts:22:11)
    at RequestExecutor.<anonymous> (src\Http\RequestExecutor.ts:965:35)
    at Generator.next (<anonymous>)
    at fulfilled (src\Http\RequestExecutor.ts:5:58)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
ayende commented 1 year ago
const dbRecord = await store.maintenance.send(getDbOp);

This should not fail, but return a null result, no? Pretty sure that this is what we do with .NET

kamranayub commented 1 year ago

@ml054

This is in the Cloudflare runtime (under wrangler) FYI, which may change things.

The full setup code looks like

const databaseName = 'CloudflareTemplate-' + crypto.randomUUID();
store = new DocumentStore(['http://live-test.ravendb.net'], databaseName);

store.conventions.customFetch = fetch;
store.conventions.disableTopologyUpdates = true;
store.initialize();

await createDbIfNotExists(store);
ml054 commented 1 year ago

@kamranayub I assume fetch has correctly bound this, right?

kamranayub commented 1 year ago

@kamranayub I assume fetch has correctly bound this, right?

I will double check that, I don't explicitly right now but maybe that's needed here too.

kamranayub commented 1 year ago

@ml054 Same result whether it's bound or not. I think it makes sense based on the code path, right?

  1. Outgoing request gets made
  2. Response comes back (successfully) as 404
  3. >= 400 status code path gets hit
  4. _handleUnsuccessfulResponse happens
  5. Checks for body stream, none exists
  6. Throws an exception
  7. Doesn't reach the part where it checks for DB missing header
ml054 commented 1 year ago

@kamranayub

I will check that in next week. Will update you.

ml054 commented 1 year ago

@kamranayub

I was curious if it works on regular db, and... it throws as well.

So it means I can have failing test w/o cloudflare.

ml054 commented 1 year ago

I reported this here: https://issues.hibernatingrhinos.com/issue/RDBC-693/Node.js-client-should-be-able-to-handle-404-in-GetDatabaseRecordOperation

As temporary workaround you can use GetDatabaseNamesOperation