Make path prefixing injectable through a strategy in S3

[frankdejonge]

No description provided.

[akrolasik-bc]

Unfortunately, it doesn't work. I tested it with this code:

const csv = 'name,age\nJohn,30\nJane,25';
const prefix = 'path/to/files';
const fileName = 'users.csv';

const storage = new FileStorage(new AwsS3StorageAdapter(
  client,
  {
    bucket: '<bucket-name>',
    prefix,
  },
  undefined,
  undefined,
  windowsPathPrefixer,
));

storage.write(fileName, csv).then(() => {
  console.log('File written using FileStorage');
});

And got the result:

The key for PutObjectCommandInput is path/to/files///users.csv for the new solution.

It's not an issue related to a system.
The issue is that there is no transformation of the path from any system to the S3 bucket index.
It works for unix just by chance.

[frankdejonge]

@akrolasik-bc the code you've tested with doesn't use the mitigation. It should be:

// ..imports
import {windowsPathPrefixer} from '@flystorage/aws-s3';
const csv = 'name,age\nJohn,30\nJane,25';
const prefix = 'path/to/files';
const fileName = 'users.csv';

const storage = new FileStorage(new AwsS3StorageAdapter(
  client,
  {
    bucket: '<bucket-name>',
    prefix,
  },
  undefined,
  undefined,
  windowsPathPrefixer,
));

storage.write(fileName, csv).then(() => {
  console.log('File written using FileStorage');
});

[akrolasik-bc]

That was easier:

readonly prefixerProvider: PathPrefixerProvider = windowsPathPrefixer,

[frankdejonge]

@akrolasik-bc easier for you, but I reckon the majority doesn't use windows and doesn't deploy on windows. With that in mind, I do not think using the windows strategy as the default is the right call.

[akrolasik-bc]

I changed the way I'm passing the service, but the issue remains the same.

1. The provided fix does not resolve the issue for Windows.
2. The library itself does not require fixing for any particular system regarding the S3 adapter; rather, the logic needs an update.
3. The AWS S3 adapter works for Unix by chance—the path separator happens to be the same in both.

Ad.2. That was written really unfortunately. By 'the logic,' I meant the logic you provided in the PR—the regex-based logic.

[frankdejonge]

@akrolasik-bc heya, please keep things constructive. Instead of saying "it doesn't work", provide information to help debugging. Saying "the logic needs an update" doesn't give me much in terms of actionable feedback either. Saying the adapter works by chance is 1) not helpful, and 2) not very respectful. I'm happy to work with you towards a resolution, but it's pretty hard to do with limited input.

[akrolasik-bc]

Sorry. I assure you I didn't mean to disrespect you. When I read my comment again, I indeed got the impression it may sound disrespectful. It's because it was written in a hurry. Also, the message might sound disrespectful if we remove all the context and all the input I have already provided you. I believe I provided all the input required for implementing a solution. I will try to explain as best as I can what I mean and what I suggest the solution should be.

Let's start with the information I provided in the issue:
#19 (comment)

AWS S3 (Amazon Simple Storage Service) is often mistaken for a file system due to its ability to store and manage data in a hierarchical structure similar to folders and files. However, there are several fundamental differences that distinguish S3 from a traditional file system. Among many differences, the important one here is that S3 is an object storage service, not a block or file-level storage system. In S3, data is stored as objects, where each object consists of data, metadata, and a unique identifier (key). There are no directories or folders in the traditional sense; what appear as folders in the S3 console are part of the object key naming convention.

When I mentioned "by chance," I was referring to the fact that the path separator characters are unrelated in Unix and S3 bucket paths but happen to be the same. I did not mean to disrespect you by suggesting anything else in the package works "by chance."

PathPrefixer is a very good idea for constructing paths dependent on the chosen storage. So, whatever separator character an AWS S3 bucket is using in their keys, flystorage should be able to easily achieve the right result. But somehow, the PathPrefixer became dependent on the local file system even though there is a way to provide a custom join function. It's because the custom join function for S3 bucket PathPrefixer uses the 'node:path' join function in its body.

My initial idea, which I mentioned in the issue and also in the PR comment, was to continue using 'node:path' because it's really convenient. However, I suggest transforming the path to the S3 bucket key format just before the path becomes the key.

But then, you came up with the windowsPathPrefixer, which doesn't depend on 'node:path.' This makes it an excellent candidate to be the defaultPathPrefixer—universal for any system and precisely tailored for S3 buckets.

Unfortunately, when I checked your change on my local fork, I noticed the result path is not right. Possibly the regex is not right and it produces the path with three slashes right after each other. The result is provided here:
#93 (comment)

Since the windowsPathPrefixer is actually not dependent on any local file system, you can test it on any system with the code I provided.

Sorry, I was really confused by your comment about 'the majority doesn't use Windows and doesn't deploy on Windows' right after you provided your system-independent solution for building S3 bucket paths (based on regex). I was wondering how that is related to the issue.

[akrolasik-bc]

I have thoroughly reviewed the code and would like to share my findings. Improving the library for the 17% of developers, as identified in the Node.js Developers Survey, requires some additional adjustments.

Since the library is readily accessible to developers using Windows without any restrictions, it is essential to either resolve the issues or provide information to alert developers about potential problems (except one issue). After examining the package description, I did not find any details regarding system dependencies, but the problems could manifest in many different ways according to what I see in the code.

There are a few enhancements that could be made to the library concerning path construction. The dependency on 'node:path' is repeated in several places, leading to a reliance on the system. Additionally, the path separator is defined in multiple locations, resulting in inconsistent control. Even when a custom join function and path separator are defined, they are often disregarded in various parts of the code.

Furthermore, the PathPrefixer seems to have two responsibilities: prefixing/stripping and converting file paths to directory paths. The second is more related to "suffixing" rather than "prefixing."

To effectively address these issues, I propose implementing some code changes, as illustrated below.
To centralize the control over path building I propose PathBuilderInterface.

export interface PathBuilderInterface {
    join(...paths: string[]): string;
    normalize(path: string): string;
    convertToDirectoryPath(path: string): string;
    convertToFilePath(path: string): string;
    ...
}

PathBuilder can still be implemented using 'node:path'; however, this time, control over the path separator is centralized. Here is a basic implementation of the PathBuilder class. It currently lacks implementation related to path delimiter, handling absolute and relative paths, managing the root, and encoding uni components. Thus far, I've focused only on the path separator issue for the S3 bucket.

export class PathBuilder {
    constructor(private readonly pathSrparator = '/') {}

    join(...paths: string[]): string {
        const componentPaths = paths.map(this.normalizePathSeparator);

        for (let i = 1; i < componentPaths.length; i++) {
            componentPaths[i] = this.removeLeadingSeparator(componentPaths[i]);
        }

        for (let i = 0; i < componentPaths.length - 1; i++) {
            componentPaths[i] = this.removeTrailingSeparator(componentPaths[i]);
        }

        return componentPaths.join(this.pathSrparator);
    }

    normalize(path: string): string {
        const pathComponents = this.normalizePathSeparator(path).split(this.pathSrparator);
        const normalizedPathComponents = [];

        let counter = 0;

        for (let i = pathComponents.length -1; i >= 0; i--) {
            const component = pathComponents[i];

            if (component === '..') {
                counter++;
            } else if (component !== '.') {
                if (counter > 0) {
                    counter--;
                } else {
                    normalizedPathComponents.unshift(component);
                }
            }
        }

        return normalizedPathComponents.join(this.pathSrparator);
    }

    convertToDirectoryPath(path: string): string {
        return this.addTraillingSeparator(path);
    }

    convertToFilePath(path: string): string {
        return this.removeTrailingSeparator(path);
    }

    private normalizePathSeparator(path: string): string {
        return path.replace(/\\/g, this.pathSrparator);
    }

    private removeTrailingSeparator(path: string): string {
        return path.endsWith(this.pathSrparator) ? path.substring(0, path.length - 1) : path;
    }

    private removeLeadingSeparator(path: string): string {
        return path.startsWith(this.pathSrparator) ? path.substring(1) : path;
    }

    private addTraillingSeparator(path: string): string {
        return path.endsWith(this.pathSrparator) ? path : `${path}${this.pathSrparator}`;
    }
}

PathPrefixer lacks "suffixing" functionality and is based on the PathBuilder.

export class PathPrefixer {
    private readonly normalizedPrefix: string = '';

    constructor(prefix = '', private readonly pathResolver: PathBuilderInterface = new PathBuilder()) {
        this.normalizedPrefix = pathResolver.normalize(prefix);
    }

    prefixPath(path: string): string {
        return this.pathResolver.join(this.normalizedPrefix, path);
    }

    stripPath(path: string): string {
        return path.substring(this.normalizedPrefix.length);
    }
}