No error is thrown when a container doesn't exist

[AlexChadwickP]

Description

Currently if you supply a non-existent containerName to winstonAzureBlob nothing will happen. The container isn't created (that I'm aware of) and there is no indication that the container should exist before hand.

Suggestion

Check if the container exists (I'm not incredibly familiar with the Azure Blob SDK but this may be relevant: https://stackoverflow.com/questions/61128794/how-do-you-check-that-a-container-exists-in-azure-blob-storage-v12).

If it doesn't exist throw an error or an exception to indicate that this is the case.

Maybe you could also provide a force optional value to the object with all the configuration that will allow the application to attempt to create the container? E.g:

winstonAzureBlob({
            account: {
                name: "myAccountName",
                key: "*****************************************************"
            },
            blobName: "nodeLogs",
            bufferLogSize: 1,
            containerName: "logFiles",
            eol: "\n",
            extension: extensions.LOG,
            level: "info",
            rotatePeriod: "YYYY-MM-DD",
            syncTimeout: 0,
            allowCreateContainer: true // (optional) default is false
        });

What do you think?

I wouldn't mind giving this a shot myself when I get the chance (I'm using this at work so I could probably do so during working hours), unless you'd rather do it yourself?

Thanks for your time!

[agmoss]

Hi @AlexChadwickP!

Thank you for this identification. I concur that there should be some better validation and reporting on the presence of the supplied container name. Please feel free to take a stab at a PR!

IMO a library like this should not create a resource without express consent from the user. The API parameter of allowCreateContainer handles this nicely.

This would look something like:

    if (allowCreateContainer && !exists) {
        await containerClient.create();
    }

    if (!allowCreateContainer && !exists) {
        throw new Error(`Container with name ${containerName} does not exist`);
    }

The natural place for this check would be within the _logToAppendBlob method. This is where the container client is created. You may need to perform this check in a function from async.

Let me know if you require any assistance with this.

Cheers,

Andrew

[AlexChadwickP]

Hi Andrew,

Thanks for the swift response, I'll fork it and attempt it today, I'll update this discussions with any questions that may arise!

Cheers,

Alex

[AlexChadwickP]

I'm having some trouble throwing an error since the code needed to check whether a container exists is async. I can't throw an error in .then() because it doesn't really do anything.

Here's what I've attempted:

async checkIfContainerExists(container: ContainerClient) {
        if (!(await container.exists())) {
            throw new Error("Container doesn't exist");
        }
    }

private _logToAppendBlob(
        tasks: Array<Data>,
        callback: async.ErrorCallback<Error>
    ) {
        debug("Try to appendblock", tasks.length);
        // nothing to log
        if (tasks.length === 0) {
            return callback();
        }
        const azClient = this.azBlobClient;
        const containerName = this.containerName;
        const containerClient = azClient.getContainerClient(containerName);

        this.checkIfContainerExists(containerClient);

        const blobName = WinstonAzureBlob.generateBlobName({
            blobName: this.blobName,
            extension: this.extension,
            rotatePeriod: this.rotatePeriod,
        });
...

But without any success. As far as I'm aware Winston isn't using the async/await API so it makes asynchronous transports hard to work with.

Have you got any ideas to work around this?

[agmoss]

Hi @AlexChadwickP

I can see how this is cumbersome. Much of this library was written when async/await was not available. However, I believe that the async/await API is supported by Winston transports. Here is an example. You could attempt a re-write using async explicitly. This would be a bit of an undertaking, though.

Another option could be to perform the check in the constructor and then mark the completion of the task with a side effect variable. There is an example of this within winston-bigquery. I am not a big fan of this approach as it lacks purity.

IMO you are proceeding along the right path given the code you are working with. Perhaps try and use the callback with .then chains.

I will dive into this myself and report back.

[agmoss]

@AlexChadwickP Marking _logToAppendBlob as async and performing the check here seems to work just fine. Let's try to proceed with an async/await syntax implementation.

[AlexChadwickP]

I mean it's not a huge library so I don't think trying to migrate towards an async/await syntax will be difficult, maybe just a bit time consuming. I'll give it another attempt when I've got some more time :)