From d671721d4ce715321e83c8c02d838ddaed3521f3 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sat, 15 Apr 2023 11:26:33 +0200 Subject: [PATCH 01/20] Improve file-write - simplify the code and remove some functions - remove unnecessary unlink before the rename this is acually better as rename is atomic so we don't lose the original file if the rename fails - delete the temp file in the case of error - factor common code for tmp filename: this allows to replace two functions with one - factor common code in file copy (findId3TagPosition): this simplifies the function - factor common code to remove id3 tag from buffer: this allows to replace two functions with one - simplify function names - split long lines - implementation is much shorter - the sync/async implementations almost only differs with this calls to sync/async APIs --- src/file-read.ts | 10 +-- src/file-write.ts | 155 +++++++++++++++++++--------------------------- src/util-file.ts | 14 ++++- 3 files changed, 79 insertions(+), 100 deletions(-) diff --git a/src/file-read.ts b/src/file-read.ts index a41f5de..0d6bfc2 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -1,6 +1,6 @@ import * as fs from 'fs' import { findId3TagPosition, getId3TagSize, Header } from './id3-tag' -import { fsReadPromise, getNextBufferSubarrayAsync, getNextBufferSubarraySync, processFile, processFileAsync } from './util-file' +import { fsReadPromise, fillBufferAsync, fillBufferSync, processFileSync, processFileAsync } from './util-file' const FileBufferSize = 20 * 1024 * 1024 @@ -9,7 +9,7 @@ type ErrorCallback = (err: Error, buffer: null) => void type Callback = SuccessCallback & ErrorCallback export function getId3TagDataFromFileSync(filepath: string): Buffer|null { - return processFile(filepath, 'r', (fileDescriptor) => { + return processFileSync(filepath, 'r', (fileDescriptor) => { const partialId3TagData = findPartialId3TagSync(fileDescriptor) return partialId3TagData ? completePartialId3TagData( fileDescriptor, @@ -35,7 +35,7 @@ export function getId3TagDataFromFileAsync(filepath: string, callback: Callback) function findPartialId3TagSync(fileDescriptor: number): Buffer|null { const buffer = Buffer.alloc(FileBufferSize) let data - while((data = getNextBufferSubarraySync(fileDescriptor, buffer, Header.size)).length > Header.size) { + while((data = fillBufferSync(fileDescriptor, buffer, Header.size)).length > Header.size) { const id3TagPosition = findId3TagPosition(data) if(id3TagPosition !== -1) { return data.subarray(id3TagPosition) @@ -48,7 +48,7 @@ function findPartialId3TagSync(fileDescriptor: number): Buffer|null { async function findPartialId3TagAsync(fileDescriptor: number): Promise { const buffer = Buffer.alloc(FileBufferSize) let data - while((data = await getNextBufferSubarrayAsync(fileDescriptor, buffer, Header.size)).length > Header.size) { + while((data = await fillBufferAsync(fileDescriptor, buffer, Header.size)).length > Header.size) { const id3TagPosition = findId3TagPosition(data) if(id3TagPosition !== -1) { return data.subarray(id3TagPosition) @@ -88,4 +88,4 @@ async function completePartialId3TagDataAsync(fileDescriptor: number, partialId3 ]) } return partialId3TagData.subarray(0, id3TagSize) -} \ No newline at end of file +} diff --git a/src/file-write.ts b/src/file-write.ts index ddba36f..7f883f9 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -1,11 +1,8 @@ import { - fsReadPromise, - fsRenamePromise, - fsUnlinkPromise, fsWritePromise, - getNextBufferSubarrayAsync, - getNextBufferSubarraySync, - processFile, + fillBufferAsync, + fillBufferSync, + processFileSync, processFileAsync } from "./util-file" import * as tmp from 'tmp' @@ -16,120 +13,94 @@ import { findId3TagPosition, getId3TagSize } from "./id3-tag" const FileBufferSize = 20 * 1024 * 1024 export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { - const tmpFile = getTmpFilePathSync(filepath) - processFile(filepath, 'r', (readFileDescriptor) => { - processFile(tmpFile, 'w', (writeFileDescriptor) => { - fs.writeSync(writeFileDescriptor, id3Tag) - streamOriginalIntoNewFileSync(readFileDescriptor, writeFileDescriptor) - }) + const tmpFilepath = tmp.tmpNameSync(getTmpFileOptions(filepath)) + processFileSync(filepath, 'r', (readFileDescriptor) => { + try { + processFileSync(tmpFilepath, 'w', (writeFileDescriptor) => { + fs.writeSync(writeFileDescriptor, id3Tag) + copyFileWithoutId3TagSync(readFileDescriptor, writeFileDescriptor) + }) + } catch(error) { + fs.unlinkSync(tmpFilepath) + throw error + } }) - fs.unlinkSync(filepath) - fs.renameSync(tmpFile, filepath) + fs.renameSync(tmpFilepath, filepath) } -export function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer, callback: (err: Error|null) => void) { - getTmpFileAsync(filepath, (err, tmpFile) => { - if(err || !tmpFile) { - return callback(err) +export function writeId3TagToFileAsync( + filepath: string, + id3Tag: Buffer, + callback: (err: Error|null) => void +) { + tmp.tmpName(getTmpFileOptions(filepath), (error, tmpFilepath) => { + if (error) { + return callback(error) } - processFileAsync(filepath, 'r', async (readFileDescriptor) => { - return processFileAsync(tmpFile, 'w', async (writeFileDescriptor) => { + processFileAsync(tmpFilepath, 'w', async (writeFileDescriptor) => { await fsWritePromise(writeFileDescriptor, id3Tag) - await streamOriginalIntoNewFileAsync(readFileDescriptor, writeFileDescriptor) + await copyFileWithoutId3TagAsync(readFileDescriptor, writeFileDescriptor) + }).catch((error) => { + fs.unlink(tmpFilepath, callback) + throw error }) - }).then(async () => { - await fsUnlinkPromise(filepath) - await fsRenamePromise(tmpFile, filepath) - callback(null) - }).catch((error) => { - callback(error) - }) - }) -} - -function getTmpFilePathSync(filepath: string): string { - const parsedPath = path.parse(filepath) - return tmp.tmpNameSync({ - tmpdir: parsedPath.dir, - template: `${parsedPath.base}.tmp-XXXXXX`, + }).then(() => { + fs.rename(tmpFilepath, filepath, callback) + }).catch(callback) }) } -function getTmpFileAsync(filepath: string, callback: tmp.TmpNameCallback) { +function getTmpFileOptions(filepath: string): tmp.TmpNameOptions { const parsedPath = path.parse(filepath) - tmp.tmpName({ + return { tmpdir: parsedPath.dir, template: `${parsedPath.base}.tmp-XXXXXX`, - }, (err, filename) => { - callback(err, filename) - }) + } } -function streamOriginalIntoNewFileSync(readFileDescriptor: number, writeFileDescriptor: number) { +function copyFileWithoutId3TagSync( + readFileDescriptor: number, + writeFileDescriptor: number +) { const buffer = Buffer.alloc(FileBufferSize) - let data - while((data = getNextBufferSubarraySync(readFileDescriptor, buffer)).length) { - const id3TagPosition = findId3TagPosition(data) - if(id3TagPosition !== -1) { - data = getBufferWithoutId3TagAndSkipSync(readFileDescriptor, data, id3TagPosition) + let readData + while((readData = fillBufferSync(readFileDescriptor, buffer)).length) { + const { data, bytesToSkip } = removeId3TagIfFound(readData) + if (bytesToSkip) { + fillBufferSync(readFileDescriptor, Buffer.alloc(bytesToSkip)) } fs.writeSync(writeFileDescriptor, data, 0, data.length, null) } } -async function streamOriginalIntoNewFileAsync(readFileDescriptor: number, writeFileDescriptor: number) { +async function copyFileWithoutId3TagAsync( + readFileDescriptor: number, + writeFileDescriptor: number +) { const buffer = Buffer.alloc(FileBufferSize) - let data - while((data = await getNextBufferSubarrayAsync(readFileDescriptor, buffer)).length) { - const id3TagPosition = findId3TagPosition(data) - if(id3TagPosition !== -1) { - data = await getBufferWithoutId3TagAndSkipAsync(readFileDescriptor, data, id3TagPosition) + let readData + while((readData = await fillBufferAsync(readFileDescriptor, buffer)).length) { + const { data, bytesToSkip } = removeId3TagIfFound(readData) + if (bytesToSkip) { + await fillBufferAsync(readFileDescriptor, Buffer.alloc(bytesToSkip)) } await fsWritePromise(writeFileDescriptor, data, 0, data.length, null) } } -function getBufferWithoutId3TagAndSkipSync(fileDescriptor: number, data: Buffer, id3TagPosition: number): Buffer { - const dataFromId3Start = data.subarray(id3TagPosition) - const id3TagSize = getId3TagSize(dataFromId3Start) - if(id3TagSize > dataFromId3Start.length) { - const missingBytesCount = id3TagSize - dataFromId3Start.length - fs.readSync( - fileDescriptor, - Buffer.alloc(missingBytesCount), - 0, - missingBytesCount, - null - ) - return data.subarray(0, id3TagPosition) +function removeId3TagIfFound(data: Buffer) { + const id3TagPosition = findId3TagPosition(data) + if (id3TagPosition === -1) { + return { data } } - - const id3TagEndPosition = id3TagPosition + id3TagSize - return Buffer.concat([ - data.subarray(0, id3TagPosition), - data.subarray(id3TagEndPosition) - ]) -} - -async function getBufferWithoutId3TagAndSkipAsync(fileDescriptor: number, data: Buffer, id3TagPosition: number): Promise { const dataFromId3Start = data.subarray(id3TagPosition) const id3TagSize = getId3TagSize(dataFromId3Start) - if(id3TagSize > dataFromId3Start.length) { - const missingBytesCount = id3TagSize - dataFromId3Start.length - await fsReadPromise( - fileDescriptor, - Buffer.alloc(missingBytesCount), - 0, - missingBytesCount, - null - ) - return data.subarray(0, id3TagPosition) + return { + data: Buffer.concat([ + data.subarray(0, id3TagPosition), + dataFromId3Start.subarray(Math.min(id3TagSize, dataFromId3Start.length)) + ]), + bytesToSkip: Math.max(0, id3TagSize - dataFromId3Start.length) } - - const id3TagEndPosition = id3TagPosition + id3TagSize - return Buffer.concat([ - data.subarray(0, id3TagPosition), - data.subarray(id3TagEndPosition) - ]) -} \ No newline at end of file +} diff --git a/src/util-file.ts b/src/util-file.ts index fbfbe23..25ce37f 100644 --- a/src/util-file.ts +++ b/src/util-file.ts @@ -8,7 +8,7 @@ export const fsWritePromise = promisify(fs.write) export const fsUnlinkPromise = promisify(fs.unlink) export const fsRenamePromise = promisify(fs.rename) -export function processFile( +export function processFileSync( filepath: string, flags: string, process: (fileDescriptor: number) => T @@ -36,7 +36,11 @@ export async function processFileAsync( } } -export function getNextBufferSubarraySync(fileDescriptor: number, buffer: Buffer, offset = 0): Buffer { +export function fillBufferSync( + fileDescriptor: number, + buffer: Buffer, + offset = 0 +): Buffer { const bytesRead = fs.readSync( fileDescriptor, buffer, @@ -47,7 +51,11 @@ export function getNextBufferSubarraySync(fileDescriptor: number, buffer: Buffer return buffer.subarray(0, bytesRead + offset) } -export async function getNextBufferSubarrayAsync(fileDescriptor: number, buffer: Buffer, offset = 0): Promise { +export async function fillBufferAsync( + fileDescriptor: number, + buffer: Buffer, + offset = 0 +): Promise { const bytesRead = (await fsReadPromise( fileDescriptor, buffer, From de1d85d1e03a380aa5f0be4f3a88d22d938ac1ca Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sat, 15 Apr 2023 11:26:54 +0200 Subject: [PATCH 02/20] Document further simplification to be done --- src/api/promises.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api/promises.ts b/src/api/promises.ts index 7d93f92..f26f2c9 100644 --- a/src/api/promises.ts +++ b/src/api/promises.ts @@ -27,6 +27,7 @@ function makePromise(callback: (settle: Settle) => void) { }) } +// TODO use util.promisify instead /** * Asynchronous API for files and buffers operations using promises. * From 12fa3e58f3e44d4901dddaa63fac6cdd377ec2b8 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sun, 16 Apr 2023 08:54:44 +0200 Subject: [PATCH 03/20] Remove the use of "tmp" library --- package.json | 4 +--- src/file-write.ts | 50 +++++++++++++++++++++-------------------------- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/package.json b/package.json index 625b631..f02f8a3 100644 --- a/package.json +++ b/package.json @@ -47,8 +47,7 @@ }, "license": "MIT", "dependencies": { - "iconv-lite": "0.6.2", - "tmp": "^0.2.1" + "iconv-lite": "0.6.2" }, "devDependencies": { "@istanbuljs/nyc-config-typescript": "^1.0.2", @@ -58,7 +57,6 @@ "@types/mocha": "^10.0.1", "@types/node": "^18.11.10", "@types/source-map-support": "^0.5.6", - "@types/tmp": "^0.2.3", "@typescript-eslint/eslint-plugin": "^5.45.0", "@typescript-eslint/parser": "^5.45.0", "chai": "^4.3.7", diff --git a/src/file-write.ts b/src/file-write.ts index 7f883f9..1b0a318 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -5,27 +5,26 @@ import { processFileSync, processFileAsync } from "./util-file" -import * as tmp from 'tmp' -import * as path from 'path' import * as fs from 'fs' import { findId3TagPosition, getId3TagSize } from "./id3-tag" +import { hrtime } from "process" const FileBufferSize = 20 * 1024 * 1024 export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { - const tmpFilepath = tmp.tmpNameSync(getTmpFileOptions(filepath)) + const tempFilepath = makeTempFilepath(filepath) processFileSync(filepath, 'r', (readFileDescriptor) => { try { - processFileSync(tmpFilepath, 'w', (writeFileDescriptor) => { + processFileSync(tempFilepath, 'w', (writeFileDescriptor) => { fs.writeSync(writeFileDescriptor, id3Tag) copyFileWithoutId3TagSync(readFileDescriptor, writeFileDescriptor) }) } catch(error) { - fs.unlinkSync(tmpFilepath) + fs.unlinkSync(tempFilepath) throw error } }) - fs.renameSync(tmpFilepath, filepath) + fs.renameSync(tempFilepath, filepath) } export function writeId3TagToFileAsync( @@ -33,30 +32,25 @@ export function writeId3TagToFileAsync( id3Tag: Buffer, callback: (err: Error|null) => void ) { - tmp.tmpName(getTmpFileOptions(filepath), (error, tmpFilepath) => { - if (error) { - return callback(error) - } - processFileAsync(filepath, 'r', async (readFileDescriptor) => { - processFileAsync(tmpFilepath, 'w', async (writeFileDescriptor) => { - await fsWritePromise(writeFileDescriptor, id3Tag) - await copyFileWithoutId3TagAsync(readFileDescriptor, writeFileDescriptor) - }).catch((error) => { - fs.unlink(tmpFilepath, callback) - throw error - }) - }).then(() => { - fs.rename(tmpFilepath, filepath, callback) - }).catch(callback) - }) + const tempFilepath = makeTempFilepath(filepath) + processFileAsync(filepath, 'r', async (readFileDescriptor) => { + processFileAsync(tempFilepath, 'w', async (writeFileDescriptor) => { + await fsWritePromise(writeFileDescriptor, id3Tag) + await copyFileWithoutId3TagAsync(readFileDescriptor, writeFileDescriptor) + }).catch((error) => { + fs.unlink(tempFilepath, callback) + throw error + }) + }).then(() => { + fs.rename(tempFilepath, filepath, callback) + }).catch(callback) } -function getTmpFileOptions(filepath: string): tmp.TmpNameOptions { - const parsedPath = path.parse(filepath) - return { - tmpdir: parsedPath.dir, - template: `${parsedPath.base}.tmp-XXXXXX`, - } +function makeTempFilepath(filepath: string) { + // A high-resolution time is required to avoid potential conflicts + // when running multiple tests in parallel for example. + // Date.now() resolution is too low. + return `${filepath}.tmp-${hrtime.bigint()}` } function copyFileWithoutId3TagSync( From 08a4827475daaa5fc953d02ca187c0b86b7bdf1c Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sun, 16 Apr 2023 08:55:06 +0200 Subject: [PATCH 04/20] Cleanup the API and more - remove support for function which do not make sense for: create, read, write, update, promises - clean up return types and throws in case of error (create with callback for example) --- src/api/create.ts | 26 +--- src/api/promises.ts | 48 +++--- src/api/read.ts | 41 ++---- src/api/update.ts | 27 ++-- src/api/write.ts | 94 ++---------- src/file-write.ts | 53 ++++--- src/types/read.ts | 28 ++++ src/types/write.ts | 8 + src/util-file.ts | 26 +++- src/util.ts | 7 + test/api/create.ts | 264 ++++++++++++++++----------------- test/api/promises.ts | 129 ++++++---------- test/api/read.ts | 344 +++++++++++++++++++++---------------------- test/api/write.ts | 149 +++++++++---------- 14 files changed, 579 insertions(+), 665 deletions(-) create mode 100644 src/types/read.ts create mode 100644 src/types/write.ts diff --git a/src/api/create.ts b/src/api/create.ts index a44e6e2..80359da 100644 --- a/src/api/create.ts +++ b/src/api/create.ts @@ -1,33 +1,11 @@ import { WriteTags } from "../types/Tags" -import { isFunction } from "../util" import { createId3Tag } from "../id3-tag" -/** - * Callback used to return a buffer with the created ID Tag. - * - * @public - */ -export type CreateCallback = - (data: Buffer) => void - /** * Creates a buffer containing an ID3 Tag and returns it. * * @public */ -export function create(tags: WriteTags): Buffer - -/** - * Creates a buffer containing an ID3 Tag and returns it via the callback. - * - * @public - */ -export function create(tags: WriteTags, callback: CreateCallback): void - -export function create(tags: WriteTags, callback?: CreateCallback) { - const id3Data = createId3Tag(tags) - if (isFunction(callback)) { - return callback(id3Data) - } - return id3Data +export function create(tags: WriteTags): Buffer { + return createId3Tag(tags) } diff --git a/src/api/promises.ts b/src/api/promises.ts index f26f2c9..182b7d3 100644 --- a/src/api/promises.ts +++ b/src/api/promises.ts @@ -1,10 +1,10 @@ import { Tags, TagIdentifiers, WriteTags } from '../types/Tags' import { Options } from '../types/Options' -import { create } from "./create" -import { read, ReadCallback } from "./read" +import { ReadCallback } from "../types/read" +import { read, } from "./read" import { removeTags } from "./remove" import { update } from "./update" -import { write, WriteCallback, WriteFileCallback } from "./write" +import { write } from "./write" type Settle = { (error: NodeJS.ErrnoException | Error, result: null): void @@ -14,7 +14,7 @@ type Settle = { function makePromise(callback: (settle: Settle) => void) { return new Promise((resolve, reject) => { callback((error, result) => { - if(error) { + if (error) { reject(error) } else { // result can't be null here according the Settle callable @@ -27,43 +27,35 @@ function makePromise(callback: (settle: Settle) => void) { }) } -// TODO use util.promisify instead /** * Asynchronous API for files and buffers operations using promises. * * @public */ export const Promises = { - create: (tags: WriteTags) => - makePromise((settle: Settle) => - create(tags, result => settle(null, result)), + write: (tags: WriteTags, filepath: string) => + makePromise(settle => write( + tags, + filepath, + (error) => error ? settle(error, null) : settle(null) + )), + update: (tags: WriteTags, filepath: string, options?: Options) => + makePromise((settle) => update( + tags, + filepath, + options ?? {}, + (error) => error ? settle(error, null) : settle(null) + ) ), - write: (tags: WriteTags, filebuffer: string | Buffer) => - makePromise((callback: WriteCallback | WriteFileCallback) => - write(tags, filebuffer, callback) - ), - update: (tags: WriteTags, filebuffer: string | Buffer, options?: Options) => - makePromise((callback: WriteCallback | WriteFileCallback) => - update(tags, filebuffer, options ?? {}, callback) - ), - read: (file: string | Buffer, options?: Options) => + read: (filepath: string, options?: Options) => makePromise((callback: ReadCallback) => - read(file, options ?? {}, callback) + read(filepath, options ?? {}, callback) ), removeTags: (filepath: string) => - makePromise((settle: Settle) => + makePromise((settle) => removeTags( filepath, (error) => error ? settle(error, null) : settle(null) ) ) } as const - -/** - * Asynchronous API for files and buffers operations using promises. - * - * @public - * @deprecated Consider using `Promises` instead as `Promise` creates conflict - * with the Javascript native promise. - */ -export { Promises as Promise } diff --git a/src/api/read.ts b/src/api/read.ts index 1c7f541..7a6b5f8 100644 --- a/src/api/read.ts +++ b/src/api/read.ts @@ -1,35 +1,12 @@ -import * as fs from 'fs' import { getTagsFromId3Tag } from '../id3-tag' import { isFunction, isString } from '../util' import { Tags, TagIdentifiers } from '../types/Tags' import { Options } from '../types/Options' -import { getId3TagDataFromFileAsync, getId3TagDataFromFileSync } from '../file-read' - -/** - * Callback signature for successful asynchronous read operation. - * - * @param tags - `TagsIdentifiers` if the `rawOnly` option was true otherwise - * `Tags` - * @public - */ -export type ReadSuccessCallback = - (error: null, tags: Tags | TagIdentifiers) => void - -/** - * Callback signatures for failing asynchronous read operation. - * - * @public - */ -export type ReadErrorCallback = - (error: NodeJS.ErrnoException | Error, tags: null) => void - -/** - * Callback signatures for asynchronous read operation. - * - * @public - */ -export type ReadCallback = - ReadSuccessCallback & ReadErrorCallback +import { + getId3TagDataFromFileAsync, + getId3TagDataFromFileSync +} from '../file-read' +import { ReadCallback } from '../types/read' /** * Reads ID3-Tags synchronously from passed buffer/filepath. @@ -39,19 +16,19 @@ export type ReadCallback = export function read(filebuffer: string | Buffer, options?: Options): Tags /** - * Reads ID3-Tags asynchronously from passed buffer/filepath. + * Reads ID3-Tags asynchronously from passed filepath. * * @public */ -export function read(filebuffer: string | Buffer, callback: ReadCallback): void +export function read(filebuffer: string, callback: ReadCallback): void /** - * Reads ID3-Tags asynchronously from passed buffer/filepath. + * Reads ID3-Tags asynchronously from passed filepath. * * @public */ export function read( - filebuffer: string | Buffer, options: Options, callback: ReadCallback + filebuffer: string, options: Options, callback: ReadCallback ): void export function read( diff --git a/src/api/update.ts b/src/api/update.ts index 0c67a77..e52c7ce 100644 --- a/src/api/update.ts +++ b/src/api/update.ts @@ -1,16 +1,18 @@ import { WriteTags } from "../types/Tags" import { Options } from "../types/Options" -import { isFunction, isString } from "../util" +import { isFunction, isString, validateString } from "../util" import { read } from "./read" import { updateTags } from '../updateTags' -import { write, WriteCallback } from "./write" +import { write } from "./write" +import { WriteCallback } from "../types/write" /** * Updates ID3-Tags from the given buffer. + * Throws in case of error. * * @public */ - export function update( +export function update( tags: WriteTags, buffer: Buffer, options?: Options @@ -18,34 +20,37 @@ import { write, WriteCallback } from "./write" /** * Updates ID3-Tags synchronously in the specified file. + * Throws in case of error. * * @public */ - export function update( +export function update( tags: WriteTags, filepath: string, options?: Options -): true | Error +): void /** * Updates ID3-Tags asynchronously in the specified file. + * throws in case of error. * * @public */ - export function update( +export function update( tags: WriteTags, - filebuffer: string | Buffer, + filepath: string, callback: WriteCallback ): void /** * Updates ID3-Tags asynchronously from the given buffer or specified file. + * throws in case of error. * * @public */ - export function update( +export function update( tags: WriteTags, - filebuffer: string | Buffer, + filepath: string, options: Options, callback: WriteCallback ): void @@ -55,7 +60,7 @@ export function update( filebuffer: string | Buffer, optionsOrCallback?: Options | WriteCallback, callback?: WriteCallback -): Buffer | true | Error | void { +): Buffer | void { const options: Options = (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} callback = @@ -64,7 +69,7 @@ export function update( const currentTags = read(filebuffer, options) const updatedTags = updateTags(tags, currentTags) if (isFunction(callback)) { - return write(updatedTags, filebuffer, callback) + return write(updatedTags, validateString(filebuffer), callback) } if (isString(filebuffer)) { return write(updatedTags, filebuffer) diff --git a/src/api/write.ts b/src/api/write.ts index 6fdc9cd..3d95556 100644 --- a/src/api/write.ts +++ b/src/api/write.ts @@ -1,40 +1,13 @@ -import * as fs from "fs" import { WriteTags } from "../types/Tags" +import { WriteCallback } from "../types/write" import { create } from "./create" import { removeTagsFromBuffer } from "./remove" -import { isFunction, isString } from "../util" +import { isFunction, isString, validateString } from "../util" import { writeId3TagToFileAsync, writeId3TagToFileSync } from "../file-write" -/** - * Callback signature for successful asynchronous update and write operations. - * - * @public - */ -export type WriteSuccessCallback = - (error: null, data: Buffer) => void - -/** - * Callback signature for failing asynchronous update and write operations. - * - * @public - */ -export type WriteErrorCallback = - (error: NodeJS.ErrnoException | Error, data: null) => void - -/** - * Callback signatures for asynchronous update and write operations. - * - * @public - */ -export type WriteCallback = - WriteSuccessCallback & WriteErrorCallback - -export type WriteFileCallback = - (error: NodeJS.ErrnoException | Error | null) => void - /** * Replaces any existing tags with the given tags in the given buffer. - * + * Throws in case of error. * @public */ export function write(tags: WriteTags, buffer: Buffer): Buffer @@ -42,76 +15,39 @@ export function write(tags: WriteTags, buffer: Buffer): Buffer /** * Replaces synchronously any existing tags with the given tags in the * specified file. - * + * Throws in case of error. * @public */ -export function write(tags: WriteTags, filepath: string): true | Error +export function write(tags: WriteTags, filepath: string): void /** * Replaces asynchronously any existing tags with the given tags in the - * given buffer. - * - * @public - */ -export function write( - tags: WriteTags, filebuffer: Buffer, callback: WriteCallback -): void - -/** - * Replaces asynchronously any existing tags with the given tags in the - * given file. - * - * @public - */ -export function write( - tags: WriteTags, filebuffer: string, callback: WriteFileCallback -): void - -/** - * Replaces asynchronously any existing tags with the given tags in the - * given buffer or specified file. - * + * specified file. * @public */ export function write( - tags: WriteTags, filebuffer: string | Buffer, callback: WriteFileCallback | WriteCallback + tags: WriteTags, filepath: string, callback: WriteCallback ): void export function write( tags: WriteTags, filebuffer: string | Buffer, - callback?: WriteCallback | WriteFileCallback -): Buffer | true | Error | void { - const tagsBuffer = create(tags) + callback?: WriteCallback +): Buffer | void { + const id3Tag = create(tags) if (isFunction(callback)) { - if (isString(filebuffer)) { - return writeAsync(tagsBuffer, filebuffer, callback as WriteFileCallback) - } - return callback(null, writeInBuffer(tagsBuffer, filebuffer)) + writeId3TagToFileAsync(validateString(filebuffer), id3Tag) + .then(() => callback(null), (error) => callback(error)) + return } if (isString(filebuffer)) { - return writeSync(tagsBuffer, filebuffer) + return writeId3TagToFileSync(filebuffer, id3Tag) } - return writeInBuffer(tagsBuffer, filebuffer) + return writeInBuffer(id3Tag, filebuffer) } function writeInBuffer(tags: Buffer, buffer: Buffer) { buffer = removeTagsFromBuffer(buffer) || buffer return Buffer.concat([tags, buffer]) } - -function writeAsync(tags: Buffer, filepath: string, callback: WriteFileCallback) { - writeId3TagToFileAsync(filepath, tags, (err) => { - callback(err) - }) -} - -function writeSync(tags: Buffer, filepath: string) { - try { - writeId3TagToFileSync(filepath, tags) - return true - } catch(error) { - return error as Error - } -} diff --git a/src/file-write.ts b/src/file-write.ts index 1b0a318..7078a54 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -3,7 +3,12 @@ import { fillBufferAsync, fillBufferSync, processFileSync, - processFileAsync + processFileAsync, + fsExistsPromise, + fsWriteFilePromise, + fsRenamePromise, + unlinkIfExistSync, + unlinkIfExist } from "./util-file" import * as fs from 'fs' import { findId3TagPosition, getId3TagSize } from "./id3-tag" @@ -12,38 +17,50 @@ import { hrtime } from "process" const FileBufferSize = 20 * 1024 * 1024 export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { + if (!fs.existsSync(filepath)) { + fs.writeFileSync(filepath, id3Tag) + return + } const tempFilepath = makeTempFilepath(filepath) processFileSync(filepath, 'r', (readFileDescriptor) => { try { processFileSync(tempFilepath, 'w', (writeFileDescriptor) => { fs.writeSync(writeFileDescriptor, id3Tag) - copyFileWithoutId3TagSync(readFileDescriptor, writeFileDescriptor) + copyFileWithoutId3TagSync( + readFileDescriptor, writeFileDescriptor + ) }) } catch(error) { - fs.unlinkSync(tempFilepath) + unlinkIfExistSync(tempFilepath) throw error } }) fs.renameSync(tempFilepath, filepath) } -export function writeId3TagToFileAsync( - filepath: string, - id3Tag: Buffer, - callback: (err: Error|null) => void -) { +export async function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer) { + if (!await fsExistsPromise(filepath)) { + await fsWriteFilePromise(filepath, id3Tag) + return + } const tempFilepath = makeTempFilepath(filepath) - processFileAsync(filepath, 'r', async (readFileDescriptor) => { - processFileAsync(tempFilepath, 'w', async (writeFileDescriptor) => { - await fsWritePromise(writeFileDescriptor, id3Tag) - await copyFileWithoutId3TagAsync(readFileDescriptor, writeFileDescriptor) - }).catch((error) => { - fs.unlink(tempFilepath, callback) + await processFileAsync(filepath, 'r', async (readFileDescriptor) => { + try { + await processFileAsync(tempFilepath, 'w', + async (writeFileDescriptor) => { + await fsWritePromise(writeFileDescriptor, id3Tag) + await copyFileWithoutId3TagAsync( + readFileDescriptor, writeFileDescriptor + ) + } + ) + } catch(error) { + await unlinkIfExist(tempFilepath) throw error - }) - }).then(() => { - fs.rename(tempFilepath, filepath, callback) - }).catch(callback) + } + + }) + await fsRenamePromise(tempFilepath, filepath) } function makeTempFilepath(filepath: string) { diff --git a/src/types/read.ts b/src/types/read.ts new file mode 100644 index 0000000..30d7f16 --- /dev/null +++ b/src/types/read.ts @@ -0,0 +1,28 @@ +import { TagIdentifiers, Tags } from "./Tags" + +/** + * Callback signature for successful asynchronous read operation. + * + * @param tags - `TagsIdentifiers` if the `rawOnly` option was true otherwise + * `Tags` + * @public + */ +export type ReadSuccessCallback = + (error: null, tags: Tags | TagIdentifiers) => void + +/** + * Callback signatures for failing asynchronous read operation. + * + * @public + */ +export type ReadErrorCallback = + (error: NodeJS.ErrnoException | Error, tags: null) => void + +/** + * Callback signatures for asynchronous read operation. + * + * @public + */ +export type ReadCallback = + ReadSuccessCallback & ReadErrorCallback + diff --git a/src/types/write.ts b/src/types/write.ts new file mode 100644 index 0000000..b324b08 --- /dev/null +++ b/src/types/write.ts @@ -0,0 +1,8 @@ +/** + * Callback signatures for asynchronous update and write operations. + * `null` indicated success. + * + * @public + */ +export type WriteCallback = + (error: NodeJS.ErrnoException | Error | null) => void diff --git a/src/util-file.ts b/src/util-file.ts index 25ce37f..51d8b26 100644 --- a/src/util-file.ts +++ b/src/util-file.ts @@ -7,6 +7,30 @@ export const fsClosePromise = promisify(fs.close) export const fsWritePromise = promisify(fs.write) export const fsUnlinkPromise = promisify(fs.unlink) export const fsRenamePromise = promisify(fs.rename) +export const fsExistsPromise = promisify(fs.exists) +export const fsWriteFilePromise = promisify(fs.writeFile) + +/** + * @returns true if the file existed + */ +export function unlinkIfExistSync(filepath: string) { + const exist = fs.existsSync(filepath) + if (exist) { + fs.unlinkSync(filepath) + } + return exist +} + +/** + * @returns true if the file existed + */ +export async function unlinkIfExist(filepath: string) { + const exist = await fsExistsPromise(filepath) + if (exist) { + await fsUnlinkPromise(filepath) + } + return exist +} export function processFileSync( filepath: string, @@ -64,4 +88,4 @@ export async function fillBufferAsync( null )).bytesRead return buffer.subarray(0, bytesRead + offset) -} \ No newline at end of file +} diff --git a/src/util.ts b/src/util.ts index d1ea567..dcf0101 100644 --- a/src/util.ts +++ b/src/util.ts @@ -9,6 +9,13 @@ export const isString = (value: unknown): value is string => export const isBuffer = (value: unknown): value is Buffer => value instanceof Buffer +export const validateString = (value: unknown): string => { + if (isString(value)) { + return value + } + throw new TypeError("A string is expected") +} + export const deduplicate = (values: T[]) => [ ...new Set(values)] /** diff --git a/test/api/create.ts b/test/api/create.ts index ea6ee56..b14bf75 100644 --- a/test/api/create.ts +++ b/test/api/create.ts @@ -3,147 +3,145 @@ import assert = require('assert') import iconv = require('iconv-lite') import { encodeSize } from '../../src/util-size' -describe('NodeID3 API', function () { - describe('#create()', function () { - it('empty tags', function () { - assert.strictEqual( - NodeID3.create({}).compare( - Buffer.from([0x49, 0x44, 0x33, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) - ), - 0 - ) - }) - it('text frames', function () { - const tags = { - ...{ - TIT2: "abcdeÜ看板かんばん", - album: "nasÖÄkdnasd", - year: "1990" - } satisfies NodeID3.WriteTags, - notfound: "notfound" - } - const buffer = NodeID3.create(tags) - const titleSize = 10 + 1 + iconv.encode(tags.TIT2, 'utf16').length - const albumSize = 10 + 1 + iconv.encode(tags.album, 'utf16').length - const yearSize = 10 + 1 + iconv.encode(tags.year, 'utf16').length - assert.strictEqual(buffer.length, - 10 + // ID3 frame header - titleSize + // TIT2 header + encoding byte + utf16 bytes + utf16 string - albumSize +// same as above for album, - yearSize - ) - // Check ID3 header - assert.ok(buffer.includes( - Buffer.concat([ - Buffer.from([0x49, 0x44, 0x33, 0x03, 0x00, 0x00]), - Buffer.from(encodeSize(titleSize + albumSize + yearSize)) - ]) - )) +describe('NodeID3.create()', function () { + it('empty tags', function () { + assert.strictEqual( + NodeID3.create({}).compare( + Buffer.from([0x49, 0x44, 0x33, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + ), + 0 + ) + }) + it('text frames', function () { + const tags = { + ...{ + TIT2: "abcdeÜ看板かんばん", + album: "nasÖÄkdnasd", + year: "1990" + } satisfies NodeID3.WriteTags, + notfound: "notfound" + } + const buffer = NodeID3.create(tags) + const titleSize = 10 + 1 + iconv.encode(tags.TIT2, 'utf16').length + const albumSize = 10 + 1 + iconv.encode(tags.album, 'utf16').length + const yearSize = 10 + 1 + iconv.encode(tags.year, 'utf16').length + assert.strictEqual(buffer.length, + 10 + // ID3 frame header + titleSize + // TIT2 header + encoding byte + utf16 bytes + utf16 string + albumSize +// same as above for album, + yearSize + ) + // Check ID3 header + assert.ok(buffer.includes( + Buffer.concat([ + Buffer.from([0x49, 0x44, 0x33, 0x03, 0x00, 0x00]), + Buffer.from(encodeSize(titleSize + albumSize + yearSize)) + ]) + )) - // Check TIT2 frame - assert.ok(buffer.includes( - Buffer.concat([ - Buffer.from([0x54, 0x49, 0x54, 0x32]), - sizeToBuffer(titleSize - 10), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - iconv.encode(tags.TIT2, 'utf16') - ]) - )) - // Check album frame - assert.ok(buffer.includes( - Buffer.concat([ - Buffer.from([0x54, 0x41, 0x4C, 0x42]), - sizeToBuffer(albumSize - 10), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - iconv.encode(tags.album, 'utf16') - ]) - )) - assert.ok(buffer.includes( - Buffer.concat([ - Buffer.from([0x54, 0x59, 0x45, 0x52]), - sizeToBuffer(yearSize - 10), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - iconv.encode(tags.year, 'utf16') - ]) - )) - }) + // Check TIT2 frame + assert.ok(buffer.includes( + Buffer.concat([ + Buffer.from([0x54, 0x49, 0x54, 0x32]), + sizeToBuffer(titleSize - 10), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + iconv.encode(tags.TIT2, 'utf16') + ]) + )) + // Check album frame + assert.ok(buffer.includes( + Buffer.concat([ + Buffer.from([0x54, 0x41, 0x4C, 0x42]), + sizeToBuffer(albumSize - 10), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + iconv.encode(tags.album, 'utf16') + ]) + )) + assert.ok(buffer.includes( + Buffer.concat([ + Buffer.from([0x54, 0x59, 0x45, 0x52]), + sizeToBuffer(yearSize - 10), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + iconv.encode(tags.year, 'utf16') + ]) + )) + }) - it('user defined text frame single value', function() { - const tags = { - userDefinedText: { - description: "abc", - value: "defg" - } - } satisfies NodeID3.WriteTags - const buffer = NodeID3.create(tags).subarray(10) - const descEncoded = iconv.encode(tags.userDefinedText.description + "\0", "UTF-16") - const valueEncoded = iconv.encode(tags.userDefinedText.value, "UTF-16") + it('user defined text frame single value', function() { + const tags = { + userDefinedText: { + description: "abc", + value: "defg" + } + } satisfies NodeID3.WriteTags + const buffer = NodeID3.create(tags).subarray(10) + const descEncoded = iconv.encode(tags.userDefinedText.description + "\0", "UTF-16") + const valueEncoded = iconv.encode(tags.userDefinedText.value, "UTF-16") - assert.strictEqual(Buffer.compare( - buffer, - Buffer.concat([ - Buffer.from([0x54, 0x58, 0x58, 0x58]), - sizeToBuffer(1 + descEncoded.length + valueEncoded.length), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - descEncoded, - valueEncoded - ]) - ), 0) - }) + assert.strictEqual(Buffer.compare( + buffer, + Buffer.concat([ + Buffer.from([0x54, 0x58, 0x58, 0x58]), + sizeToBuffer(1 + descEncoded.length + valueEncoded.length), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + descEncoded, + valueEncoded + ]) + ), 0) + }) - it('user defined text frame array', function() { - const tags = { - userDefinedText: [{ - description: "abc", - value: "defg" - }, { - description: "hij", - value: "klmn" - }] - } satisfies NodeID3.WriteTags - const buffer = NodeID3.create(tags).subarray(10) - const desc1Encoded = iconv.encode(tags.userDefinedText[0].description + "\0", "UTF-16") - const value1Encoded = iconv.encode(tags.userDefinedText[0].value, "UTF-16") - const desc2Encoded = iconv.encode(tags.userDefinedText[1].description + "\0", "UTF-16") - const value2Encoded = iconv.encode(tags.userDefinedText[1].value, "UTF-16") + it('user defined text frame array', function() { + const tags = { + userDefinedText: [{ + description: "abc", + value: "defg" + }, { + description: "hij", + value: "klmn" + }] + } satisfies NodeID3.WriteTags + const buffer = NodeID3.create(tags).subarray(10) + const desc1Encoded = iconv.encode(tags.userDefinedText[0].description + "\0", "UTF-16") + const value1Encoded = iconv.encode(tags.userDefinedText[0].value, "UTF-16") + const desc2Encoded = iconv.encode(tags.userDefinedText[1].description + "\0", "UTF-16") + const value2Encoded = iconv.encode(tags.userDefinedText[1].value, "UTF-16") - assert.strictEqual(Buffer.compare( - buffer, - Buffer.concat([ - Buffer.from([0x54, 0x58, 0x58, 0x58]), - sizeToBuffer(1 + desc1Encoded.length + value1Encoded.length), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - desc1Encoded, - value1Encoded, - Buffer.from([0x54, 0x58, 0x58, 0x58]), - sizeToBuffer(1 + desc2Encoded.length + value2Encoded.length), - Buffer.from([0x00, 0x00]), - Buffer.from([0x01]), - desc2Encoded, - value2Encoded - ]) - ), 0) - }) + assert.strictEqual(Buffer.compare( + buffer, + Buffer.concat([ + Buffer.from([0x54, 0x58, 0x58, 0x58]), + sizeToBuffer(1 + desc1Encoded.length + value1Encoded.length), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + desc1Encoded, + value1Encoded, + Buffer.from([0x54, 0x58, 0x58, 0x58]), + sizeToBuffer(1 + desc2Encoded.length + value2Encoded.length), + Buffer.from([0x00, 0x00]), + Buffer.from([0x01]), + desc2Encoded, + value2Encoded + ]) + ), 0) + }) - it('create mixed v3/v4 tag', function() { - const frameBuf = Buffer.from('4944330300000000003d5449543200000009000001fffe61006c006c00545945520000000b000001fffe3200300032003200544452430000000b000001fffe3200300032003300', 'hex') + it('create mixed v3/v4 tag', function() { + const frameBuf = Buffer.from('4944330300000000003d5449543200000009000001fffe61006c006c00545945520000000b000001fffe3200300032003200544452430000000b000001fffe3200300032003300', 'hex') - const tags = { - title: "all", - year: "2022", - recordingTime: "2023" - } satisfies NodeID3.WriteTags + const tags = { + title: "all", + year: "2022", + recordingTime: "2023" + } satisfies NodeID3.WriteTags - assert.deepStrictEqual( - NodeID3.create(tags), - frameBuf - ) - }) + assert.deepStrictEqual( + NodeID3.create(tags), + frameBuf + ) }) }) diff --git a/test/api/promises.ts b/test/api/promises.ts index 00606ea..e66c948 100644 --- a/test/api/promises.ts +++ b/test/api/promises.ts @@ -1,104 +1,61 @@ import fs = require('fs') -import * as NodeID3 from '../../index' +import { Promises } from '../../index' import chai = require('chai') import chaiAsPromised = require('chai-as-promised') +import { unlinkIfExistSync } from '../../src/util-file' const { expect } = chai chai.use(chaiAsPromised) -describe('NodeID3 API Promises', function () { - const invalidFilepath = 'should-hopefully-not-be-a-valid-file.mp3' - const testFilepath = 'write-promise-test-file.mp3' - describe('#create()', function () { - it('resolve', function () { - return expect( - NodeID3.Promises.create({}) - ).to.eventually.be.instanceOf(Buffer) - }) +describe('NodeID3.Promises', function () { + const nonExistingFile = { + path: "hopefully-non-existing-file.mp3", + name: "path on non-existing file" + } + const existingFile = { + path: "promise-test-file.mp3", + name: "path on existing file" + } + beforeEach(function() { + unlinkIfExistSync(nonExistingFile.path) + fs.writeFileSync(existingFile.path, Buffer.alloc(0)) }) - const writeTestCases = [ - ['#write()', NodeID3.Promises.write], - ['#update()', NodeID3.Promises.update] - ] as const - writeTestCases.forEach(([name, fn]) => { - describe(name, function () { - describe('with buffer', function() { - it('resolve', function () { - return expect( - fn({}, Buffer.alloc(0)) - ).to.eventually.be.instanceOf(Buffer) - }) - }) - describe('with invalid file path', function() { - it('reject', function () { - return expect( - fn({}, invalidFilepath) - ).to.eventually.be.rejectedWith(Error) - }) - }) - describe('with valid file path', function() { - before(function() { - fs.writeFileSync(testFilepath, Buffer.alloc(0)) - }) - it('resolve', function () { - return expect( - fn({}, testFilepath) - ).to.eventually.be.fulfilled - }) - after(function() { - fs.unlinkSync(testFilepath) - }) - }) - }) + afterEach(function() { + unlinkIfExistSync(nonExistingFile.path) + unlinkIfExistSync(existingFile.path) }) - describe('#read()', function () { - describe('with buffer', function() { - it('resolve', function () { + type TestCase = [ + string, + {path: string, name: string}, + (path: string) => Promise + ] + const successfulTestCases: TestCase[] = [ + ["read()", existingFile, path => Promises.read(path)], + ["write()", existingFile, path => Promises.write({}, path)], + ["write()", nonExistingFile, path => Promises.write({}, path)], + ["update()", existingFile, path => Promises.update({}, path)], + ["removeTags()", existingFile, path => Promises.removeTags(path)] + ] + successfulTestCases.forEach(([funcName, fileCase, operation]) => { + describe(`${funcName} with ${fileCase.name}`, function() { + it('should resolve', function() { return expect( - NodeID3.Promises.read(Buffer.alloc(0)) - ).to.eventually.to.have.key('raw') - }) - }) - describe('with invalid file path', function() { - it('reject', function () { - return expect( - NodeID3.Promises.read(invalidFilepath) - ).to.eventually.be.rejectedWith(Error) - }) - }) - describe('with valid file path', function() { - before(function() { - fs.writeFileSync(testFilepath, Buffer.alloc(0)) - }) - it('resolve', function () { - return expect( - NodeID3.Promises.read(testFilepath) - ).to.eventually.to.have.key('raw') - }) - after(function() { - fs.unlinkSync(testFilepath) + operation(fileCase.path) + ).to.eventually.be.fulfilled }) }) }) - describe('#removeTags()', function () { - describe('with invalid file path', function() { - it('reject', function () { + const failingTestCases: TestCase[] = [ + ["read()", nonExistingFile, path => Promises.read(path)], + ["update()", nonExistingFile, path => Promises.update({}, path)], + ["removeTags()", nonExistingFile, path => Promises.removeTags(path)] + ] + failingTestCases.forEach(([funcName, fileCase, operation]) => { + describe(`${funcName} with ${fileCase.name}`, function() { + it('should reject', function() { return expect( - NodeID3.Promises.removeTags(invalidFilepath) + operation(fileCase.path) ).to.eventually.be.rejectedWith(Error) }) }) - describe('with valid file path', function() { - before(function() { - fs.writeFileSync(testFilepath, Buffer.alloc(0)) - }) - it('resolve', function () { - return expect( - NodeID3.Promises.removeTags(testFilepath) - ).to.eventually.be.fulfilled - }) - after(function() { - fs.unlinkSync(testFilepath) - }) - }) }) }) diff --git a/test/api/read.ts b/test/api/read.ts index 03c73be..5275891 100644 --- a/test/api/read.ts +++ b/test/api/read.ts @@ -1,183 +1,181 @@ import * as NodeID3 from '../../index' import assert = require('assert') -describe('NodeID3 API', function () { - describe('#read()', function() { - it('read empty id3 tag', function() { - const frame = NodeID3.create({}) - assert.deepStrictEqual( - NodeID3.read(frame), - {raw: {}} - ) - }) - - it('read text frames id3 tag', function() { - const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) - assert.deepStrictEqual( - NodeID3.read(frame), - { title: "asdfghjÄÖP", album: "naBGZwssg", raw: { TIT2: "asdfghjÄÖP", TALB: "naBGZwssg" }} - ) - }) - - it('read tag with broken frame', function() { - const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) - frame[10] = 0x99 - assert.deepStrictEqual( - NodeID3.read(frame), - { album: "naBGZwssg", raw: { TALB: "naBGZwssg" }} - ) - }) - - it('read tag with bigger size', function() { - const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) - const newFrameSize = 127 - frame[9] = 127 - assert.ok(frame.length < newFrameSize + 10) - assert.deepStrictEqual( - NodeID3.read(frame), - { title: "asdfghjÄÖP", album: "naBGZwssg", raw: { TIT2: "asdfghjÄÖP", TALB: "naBGZwssg" }} - ) - }) - - it('read tag with smaller size', function() { - const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) - frame[9] -= 25 - assert.deepStrictEqual( - NodeID3.read(frame), - { title: "asdfghjÄÖP", raw: { TIT2: "asdfghjÄÖP" }} - ) - }) - - it('read tag with invalid size', function() { - const frame = NodeID3.create({ title: 'a' }) - frame[9] = 128 - assert.deepStrictEqual( - NodeID3.read(frame).raw, - {} - ) - }) - - it('read TXXX frame', function() { - const tags = { userDefinedText: {description: "abc", value: "deg"} } - const frame = NodeID3.create(tags) - assert.deepStrictEqual( - NodeID3.read(frame), - { - userDefinedText: [tags.userDefinedText], - raw: { - TXXX: [tags.userDefinedText] - } +describe('NodeID3.read()', function () { + it('read empty id3 tag', function() { + const frame = NodeID3.create({}) + assert.deepStrictEqual( + NodeID3.read(frame), + {raw: {}} + ) + }) + + it('read text frames id3 tag', function() { + const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) + assert.deepStrictEqual( + NodeID3.read(frame), + { title: "asdfghjÄÖP", album: "naBGZwssg", raw: { TIT2: "asdfghjÄÖP", TALB: "naBGZwssg" }} + ) + }) + + it('read tag with broken frame', function() { + const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) + frame[10] = 0x99 + assert.deepStrictEqual( + NodeID3.read(frame), + { album: "naBGZwssg", raw: { TALB: "naBGZwssg" }} + ) + }) + + it('read tag with bigger size', function() { + const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) + const newFrameSize = 127 + frame[9] = 127 + assert.ok(frame.length < newFrameSize + 10) + assert.deepStrictEqual( + NodeID3.read(frame), + { title: "asdfghjÄÖP", album: "naBGZwssg", raw: { TIT2: "asdfghjÄÖP", TALB: "naBGZwssg" }} + ) + }) + + it('read tag with smaller size', function() { + const frame = NodeID3.create({ title: "asdfghjÄÖP", album: "naBGZwssg" }) + frame[9] -= 25 + assert.deepStrictEqual( + NodeID3.read(frame), + { title: "asdfghjÄÖP", raw: { TIT2: "asdfghjÄÖP" }} + ) + }) + + it('read tag with invalid size', function() { + const frame = NodeID3.create({ title: 'a' }) + frame[9] = 128 + assert.deepStrictEqual( + NodeID3.read(frame).raw, + {} + ) + }) + + it('read TXXX frame', function() { + const tags = { userDefinedText: {description: "abc", value: "deg"} } + const frame = NodeID3.create(tags) + assert.deepStrictEqual( + NodeID3.read(frame), + { + userDefinedText: [tags.userDefinedText], + raw: { + TXXX: [tags.userDefinedText] } - ) - }) - - it('read TXXX array frame', function() { - const tags = { userDefinedText: [{description: "abc", value: "deg"}, {description: "abcd", value: "efgh"}] } - const frame = NodeID3.create(tags) - assert.deepStrictEqual( - NodeID3.read(frame), - { - userDefinedText: tags.userDefinedText, - raw: { - TXXX: tags.userDefinedText - } + } + ) + }) + + it('read TXXX array frame', function() { + const tags = { userDefinedText: [{description: "abc", value: "deg"}, {description: "abcd", value: "efgh"}] } + const frame = NodeID3.create(tags) + assert.deepStrictEqual( + NodeID3.read(frame), + { + userDefinedText: tags.userDefinedText, + raw: { + TXXX: tags.userDefinedText } - ) - }) + } + ) + }) - it('create mixed v3/v4 tag', function() { - const frameBuf = Buffer.from('494433030000000000315449543200000009000001fffe61006c006c005459455200000005000001fffe33005444524300000005000001fffe3400', 'hex') + it('create mixed v3/v4 tag', function() { + const frameBuf = Buffer.from('494433030000000000315449543200000009000001fffe61006c006c005459455200000005000001fffe33005444524300000005000001fffe3400', 'hex') - const tags = { - title: "all", - year: "3", - recordingTime: "4" - } + const tags = { + title: "all", + year: "3", + recordingTime: "4" + } + + assert.deepStrictEqual( + NodeID3.read(frameBuf, { noRaw: true }), + tags + ) + }) + + it('read exclude', function() { + const tagsWithoutTitle = { + album: "nasÖÄkdnasd", + year: "1990" + } satisfies NodeID3.WriteTags + const tags = { + TIT2: "abcdeÜ看板かんばん", + ...tagsWithoutTitle + } satisfies NodeID3.WriteTags + + const buffer = NodeID3.create(tags) + const read = NodeID3.read(buffer, { exclude: ['TIT2'], noRaw: true }) + assert.deepStrictEqual( + read, + tagsWithoutTitle + ) + }) + + it('read include', function() { + const tagsWithoutYear = { + title: "abcdeÜ看板かんばん", + album: "nasÖÄkdnasd" + } satisfies NodeID3.WriteTags + const tags = { + ...tagsWithoutYear, + year: "1990" + } satisfies NodeID3.WriteTags + + const buffer = NodeID3.create(tags) + const read = NodeID3.read(buffer, { include: ['TALB', 'TIT2'], noRaw: true }) + assert.deepStrictEqual( + read, + tagsWithoutYear + ) + }) + + it('onlyRaw', function() { + const tags = { + TIT2: "abcdeÜ看板かんばん", + TALB: "nasÖÄkdnasd" + } satisfies NodeID3.WriteTags + + const buffer = NodeID3.create(tags) + const read = NodeID3.read(buffer, { onlyRaw: true }) + assert.deepStrictEqual( + read, + tags + ) + }) + + it('noRaw', function() { + const tags = { + title: "abcdeÜ看板かんばん", + album: "nasÖÄkdnasd" + } satisfies NodeID3.WriteTags + + const buffer = NodeID3.create(tags) + const read = NodeID3.read(buffer, { noRaw: true }) + assert.deepStrictEqual( + read, + tags + ) + }) + + it('compressed frame', function() { + const frameBufV3 = Buffer.from('4944330300000000001c5449543200000011008000000005789c6328492d2e0100045e01c1', 'hex') + const frameBufV4 = Buffer.from('4944330400000000001c5449543200000011000900000005789c6328492d2e0100045e01c1', 'hex') + const tags = { TIT2: 'test' } + + assert.deepStrictEqual( + NodeID3.read(frameBufV3).raw, + tags + ) - assert.deepStrictEqual( - NodeID3.read(frameBuf, { noRaw: true }), - tags - ) - }) - - it('read exclude', function() { - const tagsWithoutTitle = { - album: "nasÖÄkdnasd", - year: "1990" - } satisfies NodeID3.WriteTags - const tags = { - TIT2: "abcdeÜ看板かんばん", - ...tagsWithoutTitle - } satisfies NodeID3.WriteTags - - const buffer = NodeID3.create(tags) - const read = NodeID3.read(buffer, { exclude: ['TIT2'], noRaw: true }) - assert.deepStrictEqual( - read, - tagsWithoutTitle - ) - }) - - it('read include', function() { - const tagsWithoutYear = { - title: "abcdeÜ看板かんばん", - album: "nasÖÄkdnasd" - } satisfies NodeID3.WriteTags - const tags = { - ...tagsWithoutYear, - year: "1990" - } satisfies NodeID3.WriteTags - - const buffer = NodeID3.create(tags) - const read = NodeID3.read(buffer, { include: ['TALB', 'TIT2'], noRaw: true }) - assert.deepStrictEqual( - read, - tagsWithoutYear - ) - }) - - it('onlyRaw', function() { - const tags = { - TIT2: "abcdeÜ看板かんばん", - TALB: "nasÖÄkdnasd" - } satisfies NodeID3.WriteTags - - const buffer = NodeID3.create(tags) - const read = NodeID3.read(buffer, { onlyRaw: true }) - assert.deepStrictEqual( - read, - tags - ) - }) - - it('noRaw', function() { - const tags = { - title: "abcdeÜ看板かんばん", - album: "nasÖÄkdnasd" - } satisfies NodeID3.WriteTags - - const buffer = NodeID3.create(tags) - const read = NodeID3.read(buffer, { noRaw: true }) - assert.deepStrictEqual( - read, - tags - ) - }) - - it('compressed frame', function() { - const frameBufV3 = Buffer.from('4944330300000000001c5449543200000011008000000005789c6328492d2e0100045e01c1', 'hex') - const frameBufV4 = Buffer.from('4944330400000000001c5449543200000011000900000005789c6328492d2e0100045e01c1', 'hex') - const tags = { TIT2: 'test' } - - assert.deepStrictEqual( - NodeID3.read(frameBufV3).raw, - tags - ) - - assert.deepStrictEqual( - NodeID3.read(frameBufV4).raw, - tags - ) - }) + assert.deepStrictEqual( + NodeID3.read(frameBufV4).raw, + tags + ) }) }) diff --git a/test/api/write.ts b/test/api/write.ts index 23d64e8..edc56b3 100644 --- a/test/api/write.ts +++ b/test/api/write.ts @@ -1,91 +1,80 @@ import * as NodeID3 from '../../index' -import assert = require('assert') -import chai = require('chai') +import { expect } from 'chai' import * as fs from 'fs' +import { unlinkIfExistSync } from '../../src/util-file' +import { promisify } from 'util' -describe('NodeID3 API', function () { - describe('#write()', function() { - const nonExistingFilepath = './hopefully-does-not-exist.mp3' - it('sync not existing filepath', function() { - chai.assert.isFalse(fs.existsSync(nonExistingFilepath)) - chai.assert.instanceOf( - NodeID3.write({}, nonExistingFilepath), Error - ) - }) - it('async not existing filepath', function() { - chai.assert.isFalse(fs.existsSync(nonExistingFilepath)) - NodeID3.write({}, nonExistingFilepath, function(err) { - if(!(err instanceof Error)) { - assert.fail("No error thrown on non-existing filepath") - } - }) - }) - - const buffer = Buffer.from([0x02, 0x06, 0x12, 0x22]) - const titleTag = { - title: "abc" - } satisfies NodeID3.WriteTags - const filepath = './testfile.mp3' - - it('sync write file without id3 tag', function() { - fs.writeFileSync(filepath, buffer, 'binary') - NodeID3.write(titleTag, filepath) - const newFileBuffer = fs.readFileSync(filepath) - fs.unlinkSync(filepath) - assert.strictEqual(Buffer.compare( - newFileBuffer, - Buffer.concat([NodeID3.create(titleTag), buffer]) - ), 0) - }) - - it('async write file without id3 tag', function(done) { - fs.writeFileSync(filepath, buffer, 'binary') - NodeID3.write(titleTag, filepath, function() { - const newFileBuffer = fs.readFileSync(filepath) - fs.unlinkSync(filepath) - if(Buffer.compare( - newFileBuffer, - Buffer.concat([NodeID3.create(titleTag), buffer]) - ) === 0) { - done() - } else { - done(new Error("buffer not the same")) - } - }) +describe('NodeID3.write()', function () { + const nonExistingFilepath = './hopefully-does-not-exist.mp3' + const testFilepath = './write-test-file.mp3' + beforeEach(function () { + unlinkIfExistSync(nonExistingFilepath) + unlinkIfExistSync(testFilepath) + }) + afterEach(function() { + unlinkIfExistSync(nonExistingFilepath) + unlinkIfExistSync(testFilepath) + }) + it('sync creates a file when non-existing', function() { + NodeID3.write({}, nonExistingFilepath) + expect(fs.existsSync(nonExistingFilepath)).to.be.true + }) + it('async creates a file when non-existing', function(done) { + NodeID3.write({}, nonExistingFilepath, function(error) { + if (error) { + done(new Error("Unexpected error")) + } else { + expect(fs.existsSync(nonExistingFilepath)).to.be.true + done() + } }) + }) - { + const titleFrame = { title: "title "} satisfies NodeID3.WriteTags + const data = Buffer.from([0x02, 0x06, 0x12, 0x22]) + const titleTag = NodeID3.create(titleFrame) + const albumTag = NodeID3.create({ album: "album" }) + const titleTagThenData = Buffer.concat([titleTag, data]) + const dataThenTitleTag = Buffer.concat([data, titleTag]) + const albumTagThenData = Buffer.concat([albumTag, data]) - const bufferWithTag = Buffer.concat([NodeID3.create(titleTag), buffer]) - const albumTag = { - album: "ix123" - } satisfies NodeID3.WriteTags + const testCases = [ + ["without file", null, titleTag], + ["file without id3 tag", data, titleTagThenData], + ["file with same id3 tag", titleTagThenData, titleTagThenData], + ["file with id3 tag after data", dataThenTitleTag, titleTagThenData], + ["file with different id3 tag", albumTagThenData, titleTagThenData], + ] as const - it('sync write file with id3 tag', function() { - fs.writeFileSync(filepath, bufferWithTag, 'binary') - NodeID3.write(albumTag, filepath) - const newFileBuffer = fs.readFileSync(filepath) - fs.unlinkSync(filepath) - assert.strictEqual(Buffer.compare( - newFileBuffer, - Buffer.concat([NodeID3.create(albumTag), buffer]) - ), 0) + testCases.forEach(([caseName, inputBuffer, expectedBuffer]) => { + it(`sync write ${caseName}`, function() { + if (inputBuffer) { + fs.writeFileSync(testFilepath, inputBuffer, 'binary') + } + NodeID3.write(titleFrame, testFilepath) + const newFileBuffer = fs.readFileSync(testFilepath) + if (Buffer.compare(newFileBuffer, expectedBuffer)) { + console.log("newFileBuffer:", newFileBuffer) + console.log("expectedBuffer:", expectedBuffer) + } + expect( + Buffer.compare(newFileBuffer, expectedBuffer) + ).to.equal(0) }) - it('async write file with id3 tag', function(done) { - fs.writeFileSync(filepath, bufferWithTag, 'binary') - NodeID3.write(albumTag, filepath, function() { - const newFileBuffer = fs.readFileSync(filepath) - fs.unlinkSync(filepath) - if(Buffer.compare( - newFileBuffer, - Buffer.concat([NodeID3.create(albumTag), buffer]) - ) === 0) { - done() - } else { - done(new Error("file written incorrectly")) - } - }) + it(`async write ${caseName}`, async function() { + if (inputBuffer) { + fs.writeFileSync(testFilepath, inputBuffer, 'binary') + } + const write = promisify(NodeID3.write) + await write(titleFrame, testFilepath) + const newFileBuffer = fs.readFileSync(testFilepath) + if (Buffer.compare(newFileBuffer, expectedBuffer)) { + console.log("newFileBuffer:", newFileBuffer) + console.log("expectedBuffer:", expectedBuffer) + } + expect( + Buffer.compare(newFileBuffer, expectedBuffer) + ).to.equal(0) }) - } }) }) From 82ca7a37c2e68d23fbe162c5ed83ae8661f78cb6 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Wed, 19 Apr 2023 23:58:19 +0200 Subject: [PATCH 05/20] Add write option file buffer size - introduce WriteOptions type - implements a new write API proposal - update tests to use file buffer size options (more tests to do to improve coverage) --- src/api/write.ts | 97 ++++++++++++++++++++++++++++++++++++++++------ src/file-read.ts | 5 ++- src/file-write.ts | 55 +++++++++++++++++++++----- src/types/write.ts | 12 ++++++ test/api/write.ts | 38 +++++++++++------- test/util.ts | 4 ++ 6 files changed, 174 insertions(+), 37 deletions(-) create mode 100644 test/util.ts diff --git a/src/api/write.ts b/src/api/write.ts index 3d95556..294d7e7 100644 --- a/src/api/write.ts +++ b/src/api/write.ts @@ -1,9 +1,9 @@ import { WriteTags } from "../types/Tags" -import { WriteCallback } from "../types/write" +import { WriteCallback, WriteOptions } from "../types/write" import { create } from "./create" import { removeTagsFromBuffer } from "./remove" import { isFunction, isString, validateString } from "../util" -import { writeId3TagToFileAsync, writeId3TagToFileSync } from "../file-write" +import { writeId3TagToFile, writeId3TagToFileSync } from "../file-write" /** * Replaces any existing tags with the given tags in the given buffer. @@ -18,7 +18,11 @@ export function write(tags: WriteTags, buffer: Buffer): Buffer * Throws in case of error. * @public */ -export function write(tags: WriteTags, filepath: string): void +export function write( + tags: WriteTags, + filepath: string, + options?: WriteOptions +): void /** * Replaces asynchronously any existing tags with the given tags in the @@ -32,22 +36,91 @@ export function write( export function write( tags: WriteTags, filebuffer: string | Buffer, - callback?: WriteCallback + optionsOrCallback?: WriteOptions | WriteCallback, + maybeCallback?: WriteCallback ): Buffer | void { - const id3Tag = create(tags) + const options = + (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} + const callback = + isFunction(optionsOrCallback) ? optionsOrCallback : maybeCallback if (isFunction(callback)) { - writeId3TagToFileAsync(validateString(filebuffer), id3Tag) - .then(() => callback(null), (error) => callback(error)) + writeInFile(tags, validateString(filebuffer), options, callback) return } if (isString(filebuffer)) { - return writeId3TagToFileSync(filebuffer, id3Tag) + return writeInFileSync(tags, filebuffer, options) } - return writeInBuffer(id3Tag, filebuffer) + return writeInBuffer(tags, filebuffer) +} + +// New API + +/** + * Replaces any existing tags with the given tags in the given buffer. + * Throws in case of error. + * @public + */ +export function writeInBuffer(tags: WriteTags, buffer: Buffer): Buffer { + const id3Tag = create(tags) + const bufferWithoutId3Tag = removeTagsFromBuffer(buffer) || buffer + return Buffer.concat([id3Tag, bufferWithoutId3Tag]) +} + +/** + * Replaces synchronously any existing tags with the given tags in the + * specified file. + * Throws in case of error. + * @public + */ +export function writeInFileSync( + tags: WriteTags, + filepath: string, + options: WriteOptions = {} +): void { + const id3Tag = create(tags) + writeId3TagToFileSync(filepath, id3Tag, options) } -function writeInBuffer(tags: Buffer, buffer: Buffer) { - buffer = removeTagsFromBuffer(buffer) || buffer - return Buffer.concat([tags, buffer]) +/** + * Replaces asynchronously any existing tags with the given tags in the + * specified file. + * @public + */ +export function writeInFile( + tags: WriteTags, + filepath: string, + callback: WriteCallback +): void + +/** + * Replaces asynchronously any existing tags with the given tags in the + * specified file. + * @public + */ +export function writeInFile( + tags: WriteTags, + filepath: string, + options: WriteOptions, + callback: WriteCallback +): void + +/** + * Replaces asynchronously any existing tags with the given tags in the + * specified file. + * @public + */ +export function writeInFile( + tags: WriteTags, + filepath: string, + optionsOrCallback: WriteOptions | WriteCallback, + maybeCallback?: WriteCallback +): void { + const options = + (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} + const callback = + isFunction(optionsOrCallback) ? optionsOrCallback : maybeCallback + + const id3Tag = create(tags) + writeId3TagToFile(filepath, id3Tag, options, callback ?? (() => { /* */ })) } diff --git a/src/file-read.ts b/src/file-read.ts index 0d6bfc2..52c99cc 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -18,7 +18,10 @@ export function getId3TagDataFromFileSync(filepath: string): Buffer|null { }) } -export function getId3TagDataFromFileAsync(filepath: string, callback: Callback) { +export function getId3TagDataFromFileAsync( + filepath: string, + callback: Callback +) { processFileAsync(filepath, 'r', async (fileDescriptor) => { const partialId3TagData = await findPartialId3TagAsync(fileDescriptor) return partialId3TagData ? completePartialId3TagDataAsync( diff --git a/src/file-write.ts b/src/file-write.ts index 7078a54..53dbb37 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -11,12 +11,18 @@ import { unlinkIfExist } from "./util-file" import * as fs from 'fs' -import { findId3TagPosition, getId3TagSize } from "./id3-tag" +import { Header, findId3TagPosition, getId3TagSize } from "./id3-tag" +import { WriteCallback, WriteOptions } from "./types/write" import { hrtime } from "process" -const FileBufferSize = 20 * 1024 * 1024 +const MinBufferSize = Header.size +const DefaultFileBufferSize = 20 * 1024 * 1024 -export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { +export function writeId3TagToFileSync( + filepath: string, + id3Tag: Buffer, + options: WriteOptions +): void { if (!fs.existsSync(filepath)) { fs.writeFileSync(filepath, id3Tag) return @@ -27,7 +33,9 @@ export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { processFileSync(tempFilepath, 'w', (writeFileDescriptor) => { fs.writeSync(writeFileDescriptor, id3Tag) copyFileWithoutId3TagSync( - readFileDescriptor, writeFileDescriptor + readFileDescriptor, + writeFileDescriptor, + getFileBufferSize(options) ) }) } catch(error) { @@ -38,7 +46,24 @@ export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) { fs.renameSync(tempFilepath, filepath) } -export async function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer) { +export function writeId3TagToFile( + filepath: string, + id3Tag: Buffer, + options: WriteOptions, + callback: WriteCallback +): void { + writeId3TagToFileAsync(filepath, id3Tag, options) + .then( + () => callback(null), + (error) => callback(error) + ) +} + +export async function writeId3TagToFileAsync( + filepath: string, + id3Tag: Buffer, + options: WriteOptions +): Promise { if (!await fsExistsPromise(filepath)) { await fsWriteFilePromise(filepath, id3Tag) return @@ -50,7 +75,9 @@ export async function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer) { async (writeFileDescriptor) => { await fsWritePromise(writeFileDescriptor, id3Tag) await copyFileWithoutId3TagAsync( - readFileDescriptor, writeFileDescriptor + readFileDescriptor, + writeFileDescriptor, + getFileBufferSize(options) ) } ) @@ -63,6 +90,12 @@ export async function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer) { await fsRenamePromise(tempFilepath, filepath) } +function getFileBufferSize(options: WriteOptions) { + return Math.max( + options.fileBufferSize ?? DefaultFileBufferSize, + MinBufferSize + ) +} function makeTempFilepath(filepath: string) { // A high-resolution time is required to avoid potential conflicts // when running multiple tests in parallel for example. @@ -72,9 +105,10 @@ function makeTempFilepath(filepath: string) { function copyFileWithoutId3TagSync( readFileDescriptor: number, - writeFileDescriptor: number + writeFileDescriptor: number, + fileBufferSize: number ) { - const buffer = Buffer.alloc(FileBufferSize) + const buffer = Buffer.alloc(fileBufferSize) let readData while((readData = fillBufferSync(readFileDescriptor, buffer)).length) { const { data, bytesToSkip } = removeId3TagIfFound(readData) @@ -87,9 +121,10 @@ function copyFileWithoutId3TagSync( async function copyFileWithoutId3TagAsync( readFileDescriptor: number, - writeFileDescriptor: number + writeFileDescriptor: number, + fileBufferSize: number ) { - const buffer = Buffer.alloc(FileBufferSize) + const buffer = Buffer.alloc(fileBufferSize) let readData while((readData = await fillBufferAsync(readFileDescriptor, buffer)).length) { const { data, bytesToSkip } = removeId3TagIfFound(readData) diff --git a/src/types/write.ts b/src/types/write.ts index b324b08..a989dca 100644 --- a/src/types/write.ts +++ b/src/types/write.ts @@ -6,3 +6,15 @@ */ export type WriteCallback = (error: NodeJS.ErrnoException | Error | null) => void + +/** + * Options for write operations. + * + * @public + */ +export type WriteOptions = { + /** + * File buffer size in bytes. + */ + fileBufferSize?: number +} diff --git a/test/api/write.ts b/test/api/write.ts index edc56b3..90f0b18 100644 --- a/test/api/write.ts +++ b/test/api/write.ts @@ -2,7 +2,7 @@ import * as NodeID3 from '../../index' import { expect } from 'chai' import * as fs from 'fs' import { unlinkIfExistSync } from '../../src/util-file' -import { promisify } from 'util' +import { createTestBuffer } from '../util' describe('NodeID3.write()', function () { const nonExistingFilepath = './hopefully-does-not-exist.mp3' @@ -31,7 +31,7 @@ describe('NodeID3.write()', function () { }) const titleFrame = { title: "title "} satisfies NodeID3.WriteTags - const data = Buffer.from([0x02, 0x06, 0x12, 0x22]) + const data = createTestBuffer(256) const titleTag = NodeID3.create(titleFrame) const albumTag = NodeID3.create({ album: "album" }) const titleTagThenData = Buffer.concat([titleTag, data]) @@ -46,12 +46,15 @@ describe('NodeID3.write()', function () { ["file with different id3 tag", albumTagThenData, titleTagThenData], ] as const + // Forces the code path to read a split tag + const fileBufferSize = Math.min(titleTag.length, data.length) - 1 + testCases.forEach(([caseName, inputBuffer, expectedBuffer]) => { it(`sync write ${caseName}`, function() { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } - NodeID3.write(titleFrame, testFilepath) + NodeID3.writeInFileSync(titleFrame, testFilepath, { fileBufferSize }) const newFileBuffer = fs.readFileSync(testFilepath) if (Buffer.compare(newFileBuffer, expectedBuffer)) { console.log("newFileBuffer:", newFileBuffer) @@ -61,20 +64,27 @@ describe('NodeID3.write()', function () { Buffer.compare(newFileBuffer, expectedBuffer) ).to.equal(0) }) - it(`async write ${caseName}`, async function() { + it(`async write ${caseName}`, function(done) { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } - const write = promisify(NodeID3.write) - await write(titleFrame, testFilepath) - const newFileBuffer = fs.readFileSync(testFilepath) - if (Buffer.compare(newFileBuffer, expectedBuffer)) { - console.log("newFileBuffer:", newFileBuffer) - console.log("expectedBuffer:", expectedBuffer) - } - expect( - Buffer.compare(newFileBuffer, expectedBuffer) - ).to.equal(0) + NodeID3.writeInFile( + titleFrame, testFilepath, { fileBufferSize }, (error) => { + if (error) { + done(error) + return + } + const newFileBuffer = fs.readFileSync(testFilepath) + if (Buffer.compare(newFileBuffer, expectedBuffer)) { + console.log("newFileBuffer:", newFileBuffer) + console.log("expectedBuffer:", expectedBuffer) + } + expect( + Buffer.compare(newFileBuffer, expectedBuffer) + ).to.equal(0) + done() + } + ) }) }) }) diff --git a/test/util.ts b/test/util.ts new file mode 100644 index 0000000..beb7a04 --- /dev/null +++ b/test/util.ts @@ -0,0 +1,4 @@ + +export function createTestBuffer(length: number): Buffer { + return Buffer.from(Array.from({ length }, (_, index) => index % 256)) +} From 5b0d804c942ec4e703704acaf0e84a805a5e05ab Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Fri, 21 Apr 2023 09:35:06 +0200 Subject: [PATCH 06/20] Add some failing write test cases --- test/api/write.ts | 113 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 33 deletions(-) diff --git a/test/api/write.ts b/test/api/write.ts index 90f0b18..3107976 100644 --- a/test/api/write.ts +++ b/test/api/write.ts @@ -30,58 +30,105 @@ describe('NodeID3.write()', function () { }) }) - const titleFrame = { title: "title "} satisfies NodeID3.WriteTags const data = createTestBuffer(256) - const titleTag = NodeID3.create(titleFrame) - const albumTag = NodeID3.create({ album: "album" }) - const titleTagThenData = Buffer.concat([titleTag, data]) - const dataThenTitleTag = Buffer.concat([data, titleTag]) - const albumTagThenData = Buffer.concat([albumTag, data]) + const tag = NodeID3.create({ album: "album"}) + const newFrame = { title: "title "} satisfies NodeID3.WriteTags + const newTag = NodeID3.create(newFrame) - const testCases = [ - ["without file", null, titleTag], - ["file without id3 tag", data, titleTagThenData], - ["file with same id3 tag", titleTagThenData, titleTagThenData], - ["file with id3 tag after data", dataThenTitleTag, titleTagThenData], - ["file with different id3 tag", albumTagThenData, titleTagThenData], - ] as const + type TestCase = [ + // Name + string, + // Input data + readonly Buffer[] | null, + // Expected data + readonly Buffer[], + // File buffer size + number | null + ] + const testCases: TestCase[] = [ + [ + "without file", + null, [newTag], null + ], + [ + "without tag (buffer.length > data.length)", + [data], [newTag, data], data.length + 1 + ], + [ + "without tag (buffer.length === data.length)", + [data], [newTag, data], data.length / 2 + ], + [ + "without tag (buffer.length < data.length)", + [data], [newTag, data], data.length - 1 + ], + [ + "with same tag", + [newTag, data], [newTag, data], newTag.length - 1 + ], + [ + "with tag at the start", + [tag, data], [newTag, data], tag.length - 1 + ], + [ + "with tag in the middle (ID3 identifier crossover)", + [data, tag, data], [newTag, data, data], data.length + 1 + ], + [ + "with tag in the middle (at ID3 identifier position)", + [data, tag, data], [newTag, data, data], data.length + ], + [ + "with tag in the middle (before ID3 identifier position)", + [data, tag, data], [newTag, data, data], data.length - 1 + ], + [ + "with multiple tags", + [data, tag, data, tag, data], [newTag, data, data, data], data.length - 1 + ], + [ + "with multiple tags (buffer smaller than tag)", + [data, tag, data, tag, data], [newTag, data, data, data], tag.length - 1 + ], + [ + "with multiple tags (2nd tag across reads)", + [data, tag, tag, data], [newTag, data, data], data.length + ], + [ + "w/ multiple tags (2nd tag across reads, buffer smalller than tag)", + [data, tag, tag, data], [newTag, data, data], tag.length - 1 + ], + [ + "with tag at the end", + [data, tag], [newTag, data], data.length - 1 + ], + ] - // Forces the code path to read a split tag - const fileBufferSize = Math.min(titleTag.length, data.length) - 1 - - testCases.forEach(([caseName, inputBuffer, expectedBuffer]) => { + testCases.forEach( + ([caseName, inputBuffers, expectedBuffers, fileBufferSize]) => { + const options = fileBufferSize ? { fileBufferSize } : {} + const inputBuffer = inputBuffers ? Buffer.concat(inputBuffers) : null + const expectedBuffer = Buffer.concat(expectedBuffers) it(`sync write ${caseName}`, function() { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } - NodeID3.writeInFileSync(titleFrame, testFilepath, { fileBufferSize }) + NodeID3.writeInFileSync(newFrame, testFilepath, options) const newFileBuffer = fs.readFileSync(testFilepath) - if (Buffer.compare(newFileBuffer, expectedBuffer)) { - console.log("newFileBuffer:", newFileBuffer) - console.log("expectedBuffer:", expectedBuffer) - } - expect( - Buffer.compare(newFileBuffer, expectedBuffer) - ).to.equal(0) + expect(newFileBuffer).to.deep.equal(expectedBuffer) }) it(`async write ${caseName}`, function(done) { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } NodeID3.writeInFile( - titleFrame, testFilepath, { fileBufferSize }, (error) => { + newFrame, testFilepath, options, (error) => { if (error) { done(error) return } const newFileBuffer = fs.readFileSync(testFilepath) - if (Buffer.compare(newFileBuffer, expectedBuffer)) { - console.log("newFileBuffer:", newFileBuffer) - console.log("expectedBuffer:", expectedBuffer) - } - expect( - Buffer.compare(newFileBuffer, expectedBuffer) - ).to.equal(0) + expect(newFileBuffer).to.deep.equal(expectedBuffer) done() } ) From 317b3149d7569679a415481292719e2196d8b29e Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sun, 23 Apr 2023 14:47:43 +0200 Subject: [PATCH 07/20] Add a getId3Tag() util function Returning this information is cheap and will simplify code in other places. --- src/id3-tag.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/id3-tag.ts b/src/id3-tag.ts index 2a69869..bd49782 100644 --- a/src/id3-tag.ts +++ b/src/id3-tag.ts @@ -204,6 +204,23 @@ function parseTagHeaderFlags(header: Buffer): TagHeaderFlags { } } +export function getId3Tag(data: Buffer) { + const position = findId3TagPosition(data) + if (position === -1) { + return null + } + const from = data.subarray(position) + const size = getId3TagSize(from) + return { + size, + from, + before: data.subarray(0, position), + data: from.subarray(0, size), + after: from.subarray(size), + missingBytes: Math.max(0, size - from.length) + } +} + /** * Returns the position of the first valid tag found or -1 if no tag was found. */ From 538bb1eff39a18b5a28944663de790dbbaaa6ecd Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sun, 23 Apr 2023 23:27:57 +0200 Subject: [PATCH 08/20] Fix write for ID3 id spanning across two read access - also simplify some names --- src/file-read.ts | 1 + src/file-write.ts | 115 ++++++++++++++++++++++++++++++---------------- src/util-file.ts | 14 ++++++ 3 files changed, 90 insertions(+), 40 deletions(-) diff --git a/src/file-read.ts b/src/file-read.ts index 52c99cc..3ec2352 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -35,6 +35,7 @@ export function getId3TagDataFromFileAsync( }) } +// Need to handle the case when the id3 tag size is larger than the buffer function findPartialId3TagSync(fileDescriptor: number): Buffer|null { const buffer = Buffer.alloc(FileBufferSize) let data diff --git a/src/file-write.ts b/src/file-write.ts index 53dbb37..95dc068 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -1,22 +1,25 @@ import { fsWritePromise, - fillBufferAsync, - fillBufferSync, processFileSync, processFileAsync, fsExistsPromise, fsWriteFilePromise, fsRenamePromise, unlinkIfExistSync, - unlinkIfExist + unlinkIfExist, + fsReadAsync } from "./util-file" import * as fs from 'fs' -import { Header, findId3TagPosition, getId3TagSize } from "./id3-tag" +import { Header, getId3Tag } from "./id3-tag" import { WriteCallback, WriteOptions } from "./types/write" import { hrtime } from "process" -const MinBufferSize = Header.size -const DefaultFileBufferSize = 20 * 1024 * 1024 +// Must be at least Header.size which is the min size to detect an ID3 header. +// Naming it help identifying the code handling it. +const RolloverBufferSize = Header.size + +const MinBufferSize = RolloverBufferSize + 1 +const DefaultFileBufferSize = RolloverBufferSize + 20 * 1024 * 1024 export function writeId3TagToFileSync( filepath: string, @@ -96,6 +99,7 @@ function getFileBufferSize(options: WriteOptions) { MinBufferSize ) } + function makeTempFilepath(filepath: string) { // A high-resolution time is required to avoid potential conflicts // when running multiple tests in parallel for example. @@ -103,20 +107,68 @@ function makeTempFilepath(filepath: string) { return `${filepath}.tmp-${hrtime.bigint()}` } +class Id3TagRemover { + buffer: Buffer + rolloverSize = 0 + continue = false + + constructor(bufferSize: number) { + // TODO enforce min buffer size here, + // i.e. bufferSize + RolloverBufferSize + 1 + this.buffer = Buffer.alloc(bufferSize) + } + + getReadBuffer() { + return this.buffer.subarray(this.rolloverSize) + } + + processReadBuffer(readSize: number) { + let data = this.buffer.subarray(0, this.rolloverSize + readSize) + + // TODO extract that to id3-tag + // Remove tags from `data` + const parts: Buffer[] = [] + let missingBytes = 0 + let tag + while((tag = getId3Tag(data))) { + parts.push(tag.before) + data = tag.after + missingBytes = tag.missingBytes + } + + // Exclude rollover window on the last part + this.rolloverSize = Math.min(RolloverBufferSize, data.length, readSize) + const rolloverStart = data.length - this.rolloverSize + const rolloverData = Buffer.from(data.subarray(rolloverStart)) + parts.push(data.subarray(0, rolloverStart)) + + const writeBuffer = Buffer.concat(parts) + + // Update rollover window + rolloverData.copy(this.buffer) + + this.continue = this.rolloverSize !==0 || missingBytes !== 0 + + return { + skipBuffer: Buffer.alloc(missingBytes), + writeBuffer + } + } +} + function copyFileWithoutId3TagSync( readFileDescriptor: number, writeFileDescriptor: number, fileBufferSize: number ) { - const buffer = Buffer.alloc(fileBufferSize) - let readData - while((readData = fillBufferSync(readFileDescriptor, buffer)).length) { - const { data, bytesToSkip } = removeId3TagIfFound(readData) - if (bytesToSkip) { - fillBufferSync(readFileDescriptor, Buffer.alloc(bytesToSkip)) - } - fs.writeSync(writeFileDescriptor, data, 0, data.length, null) - } + const remover = new Id3TagRemover(fileBufferSize) + do { + const readBuffer = remover.getReadBuffer() + const sizeRead = fs.readSync(readFileDescriptor, readBuffer) + const { skipBuffer, writeBuffer } = remover.processReadBuffer(sizeRead) + fs.readSync(readFileDescriptor, skipBuffer) + fs.writeSync(writeFileDescriptor, writeBuffer) + } while(remover.continue) } async function copyFileWithoutId3TagAsync( @@ -124,29 +176,12 @@ async function copyFileWithoutId3TagAsync( writeFileDescriptor: number, fileBufferSize: number ) { - const buffer = Buffer.alloc(fileBufferSize) - let readData - while((readData = await fillBufferAsync(readFileDescriptor, buffer)).length) { - const { data, bytesToSkip } = removeId3TagIfFound(readData) - if (bytesToSkip) { - await fillBufferAsync(readFileDescriptor, Buffer.alloc(bytesToSkip)) - } - await fsWritePromise(writeFileDescriptor, data, 0, data.length, null) - } -} - -function removeId3TagIfFound(data: Buffer) { - const id3TagPosition = findId3TagPosition(data) - if (id3TagPosition === -1) { - return { data } - } - const dataFromId3Start = data.subarray(id3TagPosition) - const id3TagSize = getId3TagSize(dataFromId3Start) - return { - data: Buffer.concat([ - data.subarray(0, id3TagPosition), - dataFromId3Start.subarray(Math.min(id3TagSize, dataFromId3Start.length)) - ]), - bytesToSkip: Math.max(0, id3TagSize - dataFromId3Start.length) - } + const remover = new Id3TagRemover(fileBufferSize) + do { + const readBuffer = remover.getReadBuffer() + const sizeRead = await fsReadAsync(readFileDescriptor, readBuffer) + const { skipBuffer, writeBuffer } = remover.processReadBuffer(sizeRead) + await fsReadAsync(readFileDescriptor, skipBuffer) + await fsWriteFilePromise(writeFileDescriptor, writeBuffer) + } while(remover.continue) } diff --git a/src/util-file.ts b/src/util-file.ts index 51d8b26..640e91c 100644 --- a/src/util-file.ts +++ b/src/util-file.ts @@ -89,3 +89,17 @@ export async function fillBufferAsync( )).bytesRead return buffer.subarray(0, bytesRead + offset) } + +export async function fsReadAsync( + fileDescriptor: number, + buffer: Buffer, + offset = 0 +): Promise { + return (await fsReadPromise( + fileDescriptor, + buffer, + offset, + buffer.length, + null + )).bytesRead +} From 85150bc256f0f9ddef9dc478827a78f4493afde4 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Sun, 23 Apr 2023 23:38:47 +0200 Subject: [PATCH 09/20] Simplify removeId3Tag() --- src/id3-tag.ts | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/src/id3-tag.ts b/src/id3-tag.ts index bd49782..cec835f 100644 --- a/src/id3-tag.ts +++ b/src/id3-tag.ts @@ -85,26 +85,10 @@ export function embedFramesInId3Tag(frames: Buffer) { /** * Remove already written ID3-Frames from a buffer */ -export function removeId3Tag(data: Buffer) { - const tagPosition = findId3TagPosition(data) - if (tagPosition === -1) { - return data - } - const encodedSize = subarray(data, tagPosition + Header.offset.size, 4) - - if (!isValidEncodedSize(encodedSize)) { - return false - } - - if (data.length >= tagPosition + Header.size) { - const size = decodeSize(encodedSize) - return Buffer.concat([ - data.subarray(0, tagPosition), - data.subarray(tagPosition + size + Header.size) - ]) - } - - return data +export function removeId3Tag(data: Buffer): Buffer { + // TODO support multiple-frames, improve tests + const tag = getId3Tag(data) + return tag ? Buffer.concat([tag.before, tag.after]) : data } export function getTagsFromId3Tag(buffer: Buffer, options: Options) { From 4e7695ecb6fa856e4a469d6bb7d947db2c6c2977 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Tue, 25 Apr 2023 21:32:11 +0200 Subject: [PATCH 10/20] Simplify getId3TagBody() --- src/id3-tag.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/id3-tag.ts b/src/id3-tag.ts index cec835f..2a84304 100644 --- a/src/id3-tag.ts +++ b/src/id3-tag.ts @@ -114,8 +114,7 @@ function getId3TagBody(buffer: Buffer) { // Copy for now, it might not be necessary, but we are not really sure for // now, will be re-assessed if we can avoid the copy. - const body = Buffer.alloc(bodySize) - tagBuffer.copy(body, 0, totalHeaderSize) + const body = Buffer.from(subarray(tagBuffer, totalHeaderSize, bodySize)) return { version: tagHeader.version.major, From 1557d866ffd640fdf07309e83cd30f22c3d15976 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Tue, 25 Apr 2023 21:33:11 +0200 Subject: [PATCH 11/20] Rename getId3TagDataFromFileAync() --- src/api/read.ts | 4 ++-- src/file-read.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api/read.ts b/src/api/read.ts index 7a6b5f8..9dc7db7 100644 --- a/src/api/read.ts +++ b/src/api/read.ts @@ -3,7 +3,7 @@ import { isFunction, isString } from '../util' import { Tags, TagIdentifiers } from '../types/Tags' import { Options } from '../types/Options' import { - getId3TagDataFromFileAsync, + getId3TagDataFromFile, getId3TagDataFromFileSync } from '../file-read' import { ReadCallback } from '../types/read' @@ -60,7 +60,7 @@ function readAsync( callback: ReadCallback ) { if (isString(filebuffer)) { - getId3TagDataFromFileAsync(filebuffer, (error, data) => { + getId3TagDataFromFile(filebuffer, (error, data) => { if(error) { callback(error, null) } else { diff --git a/src/file-read.ts b/src/file-read.ts index 3ec2352..93e2288 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -18,7 +18,7 @@ export function getId3TagDataFromFileSync(filepath: string): Buffer|null { }) } -export function getId3TagDataFromFileAsync( +export function getId3TagDataFromFile( filepath: string, callback: Callback ) { From aa8ce89d40e85e27030569f48323bb74f878bcb2 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Tue, 25 Apr 2023 21:35:03 +0200 Subject: [PATCH 12/20] Experimental: Use generator in Id3TagRemover Not really simpler, the only benefit is more scoped `let tag`. Not really worth the extra complexity. --- src/file-write.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/file-write.ts b/src/file-write.ts index 95dc068..aa85450 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -127,14 +127,15 @@ class Id3TagRemover { // TODO extract that to id3-tag // Remove tags from `data` - const parts: Buffer[] = [] let missingBytes = 0 - let tag - while((tag = getId3Tag(data))) { - parts.push(tag.before) - data = tag.after - missingBytes = tag.missingBytes - } + const parts = Array.from((function*() { + let tag + while((tag = getId3Tag(data))) { + yield tag.before + data = tag.after + missingBytes = tag.missingBytes + } + })()) // Exclude rollover window on the last part this.rolloverSize = Math.min(RolloverBufferSize, data.length, readSize) From c5549e09740e0aefc34bc008ca10275eac06c76e Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Tue, 25 Apr 2023 21:36:19 +0200 Subject: [PATCH 13/20] Simplify file write --- src/api/write.ts | 9 ++++++--- src/file-write.ts | 19 +++---------------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/src/api/write.ts b/src/api/write.ts index 294d7e7..78f92df 100644 --- a/src/api/write.ts +++ b/src/api/write.ts @@ -3,7 +3,7 @@ import { WriteCallback, WriteOptions } from "../types/write" import { create } from "./create" import { removeTagsFromBuffer } from "./remove" import { isFunction, isString, validateString } from "../util" -import { writeId3TagToFile, writeId3TagToFileSync } from "../file-write" +import { writeId3TagToFileAsync, writeId3TagToFileSync } from "../file-write" /** * Replaces any existing tags with the given tags in the given buffer. @@ -114,7 +114,7 @@ export function writeInFile( tags: WriteTags, filepath: string, optionsOrCallback: WriteOptions | WriteCallback, - maybeCallback?: WriteCallback + maybeCallback: WriteCallback = () => { /* */ } ): void { const options = (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} @@ -122,5 +122,8 @@ export function writeInFile( isFunction(optionsOrCallback) ? optionsOrCallback : maybeCallback const id3Tag = create(tags) - writeId3TagToFile(filepath, id3Tag, options, callback ?? (() => { /* */ })) + writeId3TagToFileAsync(filepath, id3Tag, options).then( + () => callback(null), + (error) => callback(error) + ) } diff --git a/src/file-write.ts b/src/file-write.ts index aa85450..aa65c4a 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -11,7 +11,7 @@ import { } from "./util-file" import * as fs from 'fs' import { Header, getId3Tag } from "./id3-tag" -import { WriteCallback, WriteOptions } from "./types/write" +import { WriteOptions } from "./types/write" import { hrtime } from "process" // Must be at least Header.size which is the min size to detect an ID3 header. @@ -49,19 +49,6 @@ export function writeId3TagToFileSync( fs.renameSync(tempFilepath, filepath) } -export function writeId3TagToFile( - filepath: string, - id3Tag: Buffer, - options: WriteOptions, - callback: WriteCallback -): void { - writeId3TagToFileAsync(filepath, id3Tag, options) - .then( - () => callback(null), - (error) => callback(error) - ) -} - export async function writeId3TagToFileAsync( filepath: string, id3Tag: Buffer, @@ -108,8 +95,8 @@ function makeTempFilepath(filepath: string) { } class Id3TagRemover { - buffer: Buffer - rolloverSize = 0 + private buffer: Buffer + private rolloverSize = 0 continue = false constructor(bufferSize: number) { From 15b71668946a2cfde1ff1c246418cb928847f93f Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 8 May 2023 00:56:13 +0200 Subject: [PATCH 14/20] wip: Refactor file-read --- src/api/read.ts | 17 +++--- src/file-read.ts | 112 +++++++---------------------------- src/file-stream-processor.ts | 55 +++++++++++++++++ 3 files changed, 86 insertions(+), 98 deletions(-) create mode 100644 src/file-stream-processor.ts diff --git a/src/api/read.ts b/src/api/read.ts index 9dc7db7..c59164c 100644 --- a/src/api/read.ts +++ b/src/api/read.ts @@ -3,7 +3,7 @@ import { isFunction, isString } from '../util' import { Tags, TagIdentifiers } from '../types/Tags' import { Options } from '../types/Options' import { - getId3TagDataFromFile, + getId3TagDataFromFileAsync, getId3TagDataFromFileSync } from '../file-read' import { ReadCallback } from '../types/read' @@ -49,7 +49,7 @@ export function read( function readSync(filebuffer: string | Buffer, options: Options) { if (isString(filebuffer)) { - filebuffer = getId3TagDataFromFileSync(filebuffer) ?? Buffer.alloc(0) + filebuffer = getId3TagDataFromFileSync(filebuffer)[0] ?? Buffer.alloc(0) } return getTagsFromId3Tag(filebuffer, options) } @@ -60,13 +60,12 @@ function readAsync( callback: ReadCallback ) { if (isString(filebuffer)) { - getId3TagDataFromFile(filebuffer, (error, data) => { - if(error) { - callback(error, null) - } else { - callback(null, getTagsFromId3Tag(data ?? Buffer.alloc(0), options)) - } - }) + getId3TagDataFromFileAsync(filebuffer).then( + (data) => callback( + null, getTagsFromId3Tag(data[0] ?? Buffer.alloc(0), options) + ), + (error) => callback(error, null) + ) } else { callback(null, getTagsFromId3Tag(filebuffer, options)) } diff --git a/src/file-read.ts b/src/file-read.ts index 93e2288..fa56923 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -1,95 +1,29 @@ import * as fs from 'fs' -import { findId3TagPosition, getId3TagSize, Header } from './id3-tag' -import { fsReadPromise, fillBufferAsync, fillBufferSync, processFileSync, processFileAsync } from './util-file' - -const FileBufferSize = 20 * 1024 * 1024 - -type SuccessCallback = (err: null, buffer: Buffer|null) => void -type ErrorCallback = (err: Error, buffer: null) => void -type Callback = SuccessCallback & ErrorCallback - -export function getId3TagDataFromFileSync(filepath: string): Buffer|null { - return processFileSync(filepath, 'r', (fileDescriptor) => { - const partialId3TagData = findPartialId3TagSync(fileDescriptor) - return partialId3TagData ? completePartialId3TagData( - fileDescriptor, - partialId3TagData - ) : null +import { Id3TagStreamProcessor } from './file-stream-processor' +import { processFileSync, processFileAsync, fsReadAsync } from './util-file' + +export function getId3TagDataFromFileSync(filepath: string) { + const reader = new Id3TagStreamProcessor(12) + processFileSync(filepath, 'r', (fileDescriptor) => { + do { + const readBuffer = reader.getReadBuffer() + const sizeRead = fs.readSync(fileDescriptor, readBuffer) + const missingData = reader.processReadBuffer(sizeRead) + fs.readSync(fileDescriptor, missingData) + } while(reader.continue) }) + return reader.getTags() } -export function getId3TagDataFromFile( - filepath: string, - callback: Callback -) { - processFileAsync(filepath, 'r', async (fileDescriptor) => { - const partialId3TagData = await findPartialId3TagAsync(fileDescriptor) - return partialId3TagData ? completePartialId3TagDataAsync( - fileDescriptor, - partialId3TagData - ) : null - }).then((data) => { - callback(null, data) - }).catch((error) => { - callback(error, null) +export async function getId3TagDataFromFileAsync(filepath: string) { + const reader = new Id3TagStreamProcessor(12) + await processFileAsync(filepath, 'r', async (fileDescriptor) => { + do { + const readBuffer = reader.getReadBuffer() + const sizeRead = await fsReadAsync(fileDescriptor, readBuffer) + const missingData = reader.processReadBuffer(sizeRead) + await fsReadAsync(fileDescriptor, missingData) + } while(reader.continue) }) -} - -// Need to handle the case when the id3 tag size is larger than the buffer -function findPartialId3TagSync(fileDescriptor: number): Buffer|null { - const buffer = Buffer.alloc(FileBufferSize) - let data - while((data = fillBufferSync(fileDescriptor, buffer, Header.size)).length > Header.size) { - const id3TagPosition = findId3TagPosition(data) - if(id3TagPosition !== -1) { - return data.subarray(id3TagPosition) - } - buffer.copyWithin(0, buffer.length - Header.size) - } - return null -} - -async function findPartialId3TagAsync(fileDescriptor: number): Promise { - const buffer = Buffer.alloc(FileBufferSize) - let data - while((data = await fillBufferAsync(fileDescriptor, buffer, Header.size)).length > Header.size) { - const id3TagPosition = findId3TagPosition(data) - if(id3TagPosition !== -1) { - return data.subarray(id3TagPosition) - } - buffer.copyWithin(0, buffer.length - Header.size) - } - return null -} - -function calculateMissingBytes(id3TagSize: number, id3TagBuffer: Buffer): number { - return Math.max(0, id3TagSize - id3TagBuffer.length) -} - -function completePartialId3TagData(fileDescriptor: number, partialId3TagData: Buffer): Buffer { - const id3TagSize = getId3TagSize(partialId3TagData) - const missingBytesCount = calculateMissingBytes(id3TagSize, partialId3TagData) - if(missingBytesCount) { - const id3TagRemainingBuffer = Buffer.alloc(missingBytesCount, 0x00) - fs.readSync(fileDescriptor, id3TagRemainingBuffer) - return Buffer.concat([ - partialId3TagData, - id3TagRemainingBuffer - ]) - } - return partialId3TagData.subarray(0, id3TagSize) -} - -async function completePartialId3TagDataAsync(fileDescriptor: number, partialId3TagData: Buffer): Promise { - const id3TagSize = getId3TagSize(partialId3TagData) - const missingBytesCount = calculateMissingBytes(id3TagSize, partialId3TagData) - if(missingBytesCount) { - const id3TagRemainingBuffer = Buffer.alloc(missingBytesCount, 0x00) - await fsReadPromise(fileDescriptor, {buffer: id3TagRemainingBuffer}) - return Buffer.concat([ - partialId3TagData, - id3TagRemainingBuffer - ]) - } - return partialId3TagData.subarray(0, id3TagSize) + return reader.getTags() } diff --git a/src/file-stream-processor.ts b/src/file-stream-processor.ts new file mode 100644 index 0000000..0b3deda --- /dev/null +++ b/src/file-stream-processor.ts @@ -0,0 +1,55 @@ +import { getId3Tag, Header } from './id3-tag' + +// const FileBufferSize = 20 * 1024 * 1024 + +// Must be at least Header.size which is the min size to detect an ID3 header. +// Naming it help identifying the code handling it. +const RolloverBufferSize = Header.size + +export class Id3TagStreamProcessor { + private buffer: Buffer + private tags: Buffer[] = [] + private rolloverSize = 0 + continue = false + + constructor(bufferSize: number) { + // TODO enforce min buffer size here, + // i.e. bufferSize + RolloverBufferSize + 1 + this.buffer = Buffer.alloc(bufferSize) + } + + getReadBuffer() { + return this.buffer.subarray(this.rolloverSize) + } + + processReadBuffer(readSize: number) { + let data = this.buffer.subarray(0, this.rolloverSize + readSize) + + // TODO extract that to id3-tag + // Remove tags from `data` + let missingData = Buffer.alloc(0) + let tag + while((tag = getId3Tag(data))) { + const tagBuffer = Buffer.alloc(tag.size) + tag.data.copy(tagBuffer) + data = tag.after + missingData = tagBuffer.subarray(tag.data.length) + } + + // Exclude rollover window on the last part + this.rolloverSize = Math.min(RolloverBufferSize, data.length, readSize) + const rolloverStart = data.length - this.rolloverSize + const rolloverData = Buffer.from(data.subarray(rolloverStart)) + + // Update rollover window + rolloverData.copy(this.buffer) + + this.continue = this.rolloverSize !==0 || missingData.length !== 0 + + return missingData + } + + getTags() { + return this.tags + } +} From 356333421f14661a92a4611d4215142789779e0a Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 8 May 2023 01:27:01 +0200 Subject: [PATCH 15/20] wip: Factor file stream processors --- src/file-stream-processor.ts | 50 ++++++++++++++++++++++++++++++++++ src/file-write.ts | 53 ++---------------------------------- 2 files changed, 52 insertions(+), 51 deletions(-) diff --git a/src/file-stream-processor.ts b/src/file-stream-processor.ts index 0b3deda..18b2880 100644 --- a/src/file-stream-processor.ts +++ b/src/file-stream-processor.ts @@ -53,3 +53,53 @@ export class Id3TagStreamProcessor { return this.tags } } + +export class Id3TagRemover { + private buffer: Buffer + private rolloverSize = 0 + continue = false + + constructor(bufferSize: number) { + // TODO enforce min buffer size here, + // i.e. bufferSize + RolloverBufferSize + 1 + this.buffer = Buffer.alloc(bufferSize) + } + + getReadBuffer() { + return this.buffer.subarray(this.rolloverSize) + } + + processReadBuffer(readSize: number) { + let data = this.buffer.subarray(0, this.rolloverSize + readSize) + + // TODO extract that to id3-tag + // Remove tags from `data` + let missingBytes = 0 + const parts = Array.from((function*() { + let tag + while((tag = getId3Tag(data))) { + yield tag.before + data = tag.after + missingBytes = tag.missingBytes + } + })()) + + // Exclude rollover window on the last part + this.rolloverSize = Math.min(RolloverBufferSize, data.length, readSize) + const rolloverStart = data.length - this.rolloverSize + const rolloverData = Buffer.from(data.subarray(rolloverStart)) + parts.push(data.subarray(0, rolloverStart)) + + const writeBuffer = Buffer.concat(parts) + + // Update rollover window + rolloverData.copy(this.buffer) + + this.continue = this.rolloverSize !==0 || missingBytes !== 0 + + return { + skipBuffer: Buffer.alloc(missingBytes), + writeBuffer + } + } +} diff --git a/src/file-write.ts b/src/file-write.ts index aa65c4a..dcaf0b4 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -10,9 +10,10 @@ import { fsReadAsync } from "./util-file" import * as fs from 'fs' -import { Header, getId3Tag } from "./id3-tag" +import { Header } from "./id3-tag" import { WriteOptions } from "./types/write" import { hrtime } from "process" +import { Id3TagRemover } from "./file-stream-processor" // Must be at least Header.size which is the min size to detect an ID3 header. // Naming it help identifying the code handling it. @@ -94,56 +95,6 @@ function makeTempFilepath(filepath: string) { return `${filepath}.tmp-${hrtime.bigint()}` } -class Id3TagRemover { - private buffer: Buffer - private rolloverSize = 0 - continue = false - - constructor(bufferSize: number) { - // TODO enforce min buffer size here, - // i.e. bufferSize + RolloverBufferSize + 1 - this.buffer = Buffer.alloc(bufferSize) - } - - getReadBuffer() { - return this.buffer.subarray(this.rolloverSize) - } - - processReadBuffer(readSize: number) { - let data = this.buffer.subarray(0, this.rolloverSize + readSize) - - // TODO extract that to id3-tag - // Remove tags from `data` - let missingBytes = 0 - const parts = Array.from((function*() { - let tag - while((tag = getId3Tag(data))) { - yield tag.before - data = tag.after - missingBytes = tag.missingBytes - } - })()) - - // Exclude rollover window on the last part - this.rolloverSize = Math.min(RolloverBufferSize, data.length, readSize) - const rolloverStart = data.length - this.rolloverSize - const rolloverData = Buffer.from(data.subarray(rolloverStart)) - parts.push(data.subarray(0, rolloverStart)) - - const writeBuffer = Buffer.concat(parts) - - // Update rollover window - rolloverData.copy(this.buffer) - - this.continue = this.rolloverSize !==0 || missingBytes !== 0 - - return { - skipBuffer: Buffer.alloc(missingBytes), - writeBuffer - } - } -} - function copyFileWithoutId3TagSync( readFileDescriptor: number, writeFileDescriptor: number, From 9d7c243dc0ece41bc56a3911530f23c5e35f4cc0 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 8 May 2023 11:26:15 +0200 Subject: [PATCH 16/20] Remove unused file functions --- src/util-file.ts | 58 ++++++++++++------------------------------------ 1 file changed, 14 insertions(+), 44 deletions(-) diff --git a/src/util-file.ts b/src/util-file.ts index 640e91c..4140cf9 100644 --- a/src/util-file.ts +++ b/src/util-file.ts @@ -10,6 +10,20 @@ export const fsRenamePromise = promisify(fs.rename) export const fsExistsPromise = promisify(fs.exists) export const fsWriteFilePromise = promisify(fs.writeFile) +export async function fsReadAsync( + fileDescriptor: number, + buffer: Buffer, + offset = 0 +): Promise { + return (await fsReadPromise( + fileDescriptor, + buffer, + offset, + buffer.length, + null + )).bytesRead +} + /** * @returns true if the file existed */ @@ -59,47 +73,3 @@ export async function processFileAsync( await fsClosePromise(fileDescriptor) } } - -export function fillBufferSync( - fileDescriptor: number, - buffer: Buffer, - offset = 0 -): Buffer { - const bytesRead = fs.readSync( - fileDescriptor, - buffer, - offset, - buffer.length - offset, - null - ) - return buffer.subarray(0, bytesRead + offset) -} - -export async function fillBufferAsync( - fileDescriptor: number, - buffer: Buffer, - offset = 0 -): Promise { - const bytesRead = (await fsReadPromise( - fileDescriptor, - buffer, - offset, - buffer.length - offset, - null - )).bytesRead - return buffer.subarray(0, bytesRead + offset) -} - -export async function fsReadAsync( - fileDescriptor: number, - buffer: Buffer, - offset = 0 -): Promise { - return (await fsReadPromise( - fileDescriptor, - buffer, - offset, - buffer.length, - null - )).bytesRead -} From e7ae0a149f9a1bb8cad9a15323254ea70376f5c5 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 8 May 2023 11:29:26 +0200 Subject: [PATCH 17/20] Move makeTempFilepath to util file --- src/file-write.ts | 11 ++--------- src/util-file.ts | 8 ++++++++ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/file-write.ts b/src/file-write.ts index dcaf0b4..0fbdad9 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -7,12 +7,12 @@ import { fsRenamePromise, unlinkIfExistSync, unlinkIfExist, - fsReadAsync + fsReadAsync, + makeTempFilepath } from "./util-file" import * as fs from 'fs' import { Header } from "./id3-tag" import { WriteOptions } from "./types/write" -import { hrtime } from "process" import { Id3TagRemover } from "./file-stream-processor" // Must be at least Header.size which is the min size to detect an ID3 header. @@ -88,13 +88,6 @@ function getFileBufferSize(options: WriteOptions) { ) } -function makeTempFilepath(filepath: string) { - // A high-resolution time is required to avoid potential conflicts - // when running multiple tests in parallel for example. - // Date.now() resolution is too low. - return `${filepath}.tmp-${hrtime.bigint()}` -} - function copyFileWithoutId3TagSync( readFileDescriptor: number, writeFileDescriptor: number, diff --git a/src/util-file.ts b/src/util-file.ts index 4140cf9..f061b6d 100644 --- a/src/util-file.ts +++ b/src/util-file.ts @@ -1,5 +1,6 @@ import * as fs from 'fs' import { promisify } from 'util' +import { hrtime } from "process" export const fsOpenPromise = promisify(fs.open) export const fsReadPromise = promisify(fs.read) @@ -73,3 +74,10 @@ export async function processFileAsync( await fsClosePromise(fileDescriptor) } } + +export function makeTempFilepath(filepath: string) { + // A high-resolution time is required to avoid potential conflicts + // when running multiple tests in parallel for example. + // Date.now() resolution is too low. + return `${filepath}.tmp-${hrtime.bigint()}` +} From 53eb4db9146e1c78de4ab65f8fde1208dedd3c8a Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 8 May 2023 11:30:11 +0200 Subject: [PATCH 18/20] Move buffer size defaulting to stream processor --- src/file-stream-processor.ts | 14 ++++++++++++-- src/file-write.ts | 23 ++++------------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/file-stream-processor.ts b/src/file-stream-processor.ts index 18b2880..ebaba9e 100644 --- a/src/file-stream-processor.ts +++ b/src/file-stream-processor.ts @@ -6,6 +6,9 @@ import { getId3Tag, Header } from './id3-tag' // Naming it help identifying the code handling it. const RolloverBufferSize = Header.size +const MinBufferSize = RolloverBufferSize + 1 +const DefaultFileBufferSize = RolloverBufferSize + 20 * 1024 * 1024 + export class Id3TagStreamProcessor { private buffer: Buffer private tags: Buffer[] = [] @@ -54,15 +57,22 @@ export class Id3TagStreamProcessor { } } +function getBufferSize(bufferSize?: number) { + return Math.max( + bufferSize ?? DefaultFileBufferSize, + MinBufferSize + ) +} + export class Id3TagRemover { private buffer: Buffer private rolloverSize = 0 continue = false - constructor(bufferSize: number) { + constructor(bufferSize?: number) { // TODO enforce min buffer size here, // i.e. bufferSize + RolloverBufferSize + 1 - this.buffer = Buffer.alloc(bufferSize) + this.buffer = Buffer.alloc(getBufferSize(bufferSize)) } getReadBuffer() { diff --git a/src/file-write.ts b/src/file-write.ts index 0fbdad9..eb245f1 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -11,17 +11,9 @@ import { makeTempFilepath } from "./util-file" import * as fs from 'fs' -import { Header } from "./id3-tag" import { WriteOptions } from "./types/write" import { Id3TagRemover } from "./file-stream-processor" -// Must be at least Header.size which is the min size to detect an ID3 header. -// Naming it help identifying the code handling it. -const RolloverBufferSize = Header.size - -const MinBufferSize = RolloverBufferSize + 1 -const DefaultFileBufferSize = RolloverBufferSize + 20 * 1024 * 1024 - export function writeId3TagToFileSync( filepath: string, id3Tag: Buffer, @@ -39,7 +31,7 @@ export function writeId3TagToFileSync( copyFileWithoutId3TagSync( readFileDescriptor, writeFileDescriptor, - getFileBufferSize(options) + options.fileBufferSize ) }) } catch(error) { @@ -68,7 +60,7 @@ export async function writeId3TagToFileAsync( await copyFileWithoutId3TagAsync( readFileDescriptor, writeFileDescriptor, - getFileBufferSize(options) + options.fileBufferSize ) } ) @@ -81,17 +73,10 @@ export async function writeId3TagToFileAsync( await fsRenamePromise(tempFilepath, filepath) } -function getFileBufferSize(options: WriteOptions) { - return Math.max( - options.fileBufferSize ?? DefaultFileBufferSize, - MinBufferSize - ) -} - function copyFileWithoutId3TagSync( readFileDescriptor: number, writeFileDescriptor: number, - fileBufferSize: number + fileBufferSize?: number ) { const remover = new Id3TagRemover(fileBufferSize) do { @@ -106,7 +91,7 @@ function copyFileWithoutId3TagSync( async function copyFileWithoutId3TagAsync( readFileDescriptor: number, writeFileDescriptor: number, - fileBufferSize: number + fileBufferSize?: number ) { const remover = new Id3TagRemover(fileBufferSize) do { From 57814bc3368a16ca64deb1959085656693dcbe50 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 22 May 2023 22:20:29 +0200 Subject: [PATCH 19/20] Rename write to file --- src/api/write.ts | 19 ++++++++++++------- test/api/write.ts | 4 ++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/api/write.ts b/src/api/write.ts index 78f92df..6c674ac 100644 --- a/src/api/write.ts +++ b/src/api/write.ts @@ -8,6 +8,7 @@ import { writeId3TagToFileAsync, writeId3TagToFileSync } from "../file-write" /** * Replaces any existing tags with the given tags in the given buffer. * Throws in case of error. + * @deprecated Use `writeInBuffer` instead. * @public */ export function write(tags: WriteTags, buffer: Buffer): Buffer @@ -16,6 +17,7 @@ export function write(tags: WriteTags, buffer: Buffer): Buffer * Replaces synchronously any existing tags with the given tags in the * specified file. * Throws in case of error. + * @deprecated Use `writeInFileSync` instead. * @public */ export function write( @@ -27,10 +29,13 @@ export function write( /** * Replaces asynchronously any existing tags with the given tags in the * specified file. + * @deprecated Use `writeInFile` instead. * @public */ export function write( - tags: WriteTags, filepath: string, callback: WriteCallback + tags: WriteTags, + filepath: string, + callback: WriteCallback ): void export function write( @@ -45,11 +50,11 @@ export function write( isFunction(optionsOrCallback) ? optionsOrCallback : maybeCallback if (isFunction(callback)) { - writeInFile(tags, validateString(filebuffer), options, callback) + writeToFile(tags, validateString(filebuffer), options, callback) return } if (isString(filebuffer)) { - return writeInFileSync(tags, filebuffer, options) + return writeToFileSync(tags, filebuffer, options) } return writeInBuffer(tags, filebuffer) } @@ -73,7 +78,7 @@ export function writeInBuffer(tags: WriteTags, buffer: Buffer): Buffer { * Throws in case of error. * @public */ -export function writeInFileSync( +export function writeToFileSync( tags: WriteTags, filepath: string, options: WriteOptions = {} @@ -87,7 +92,7 @@ export function writeInFileSync( * specified file. * @public */ -export function writeInFile( +export function writeToFile( tags: WriteTags, filepath: string, callback: WriteCallback @@ -98,7 +103,7 @@ export function writeInFile( * specified file. * @public */ -export function writeInFile( +export function writeToFile( tags: WriteTags, filepath: string, options: WriteOptions, @@ -110,7 +115,7 @@ export function writeInFile( * specified file. * @public */ -export function writeInFile( +export function writeToFile( tags: WriteTags, filepath: string, optionsOrCallback: WriteOptions | WriteCallback, diff --git a/test/api/write.ts b/test/api/write.ts index 3107976..49d66c7 100644 --- a/test/api/write.ts +++ b/test/api/write.ts @@ -113,7 +113,7 @@ describe('NodeID3.write()', function () { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } - NodeID3.writeInFileSync(newFrame, testFilepath, options) + NodeID3.writeToFileSync(newFrame, testFilepath, options) const newFileBuffer = fs.readFileSync(testFilepath) expect(newFileBuffer).to.deep.equal(expectedBuffer) }) @@ -121,7 +121,7 @@ describe('NodeID3.write()', function () { if (inputBuffer) { fs.writeFileSync(testFilepath, inputBuffer, 'binary') } - NodeID3.writeInFile( + NodeID3.writeToFile( newFrame, testFilepath, options, (error) => { if (error) { done(error) From 6414ad2c049d49599fcc37ba8d9e568c0b66cd70 Mon Sep 17 00:00:00 2001 From: Patrick Bricout Date: Mon, 22 May 2023 23:28:10 +0200 Subject: [PATCH 20/20] Refactor options and add read new API - add file buffer size in read options --- src/api/read.ts | 109 ++++++++++++++++++++++++++++++----- src/api/update.ts | 5 +- src/file-read.ts | 15 +++-- src/file-stream-processor.ts | 8 +-- src/file-write.ts | 2 +- src/types/FileOptions.ts | 11 ++++ src/types/read.ts | 3 + src/types/write.ts | 14 ++--- 8 files changed, 129 insertions(+), 38 deletions(-) create mode 100644 src/types/FileOptions.ts diff --git a/src/api/read.ts b/src/api/read.ts index c59164c..65c7e95 100644 --- a/src/api/read.ts +++ b/src/api/read.ts @@ -6,37 +6,44 @@ import { getId3TagDataFromFileAsync, getId3TagDataFromFileSync } from '../file-read' -import { ReadCallback } from '../types/read' +import { FileReadOptions, ReadCallback } from '../types/read' /** - * Reads ID3-Tags synchronously from passed buffer/filepath. - * + * Reads ID3-Tags from a buffer. + * @deprecated Use `readFromBuffer` instead. + * @public + */ +export function read(buffer: Buffer, options?: Options): Tags + +/** + * Reads ID3-Tags synchronously from a file. + * @deprecated Use `readFromFileSync` instead. * @public */ -export function read(filebuffer: string | Buffer, options?: Options): Tags +export function read(filepath: string, options?: FileReadOptions): Tags /** * Reads ID3-Tags asynchronously from passed filepath. - * + * @deprecated Use `readFromFile` instead. * @public */ export function read(filebuffer: string, callback: ReadCallback): void /** * Reads ID3-Tags asynchronously from passed filepath. - * + * @deprecated Use `readFromFile` instead. * @public */ export function read( - filebuffer: string, options: Options, callback: ReadCallback + filebuffer: string, options: FileReadOptions, callback: ReadCallback ): void export function read( filebuffer: string | Buffer, - optionsOrCallback?: Options | ReadCallback, + optionsOrCallback?: FileReadOptions | ReadCallback, callback?: ReadCallback ): Tags | TagIdentifiers | void { - const options: Options = + const options: FileReadOptions = (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} callback = isFunction(optionsOrCallback) ? optionsOrCallback : callback @@ -47,22 +54,22 @@ export function read( return readSync(filebuffer, options) } -function readSync(filebuffer: string | Buffer, options: Options) { +function readSync(filebuffer: string | Buffer, options: FileReadOptions) { if (isString(filebuffer)) { - filebuffer = getId3TagDataFromFileSync(filebuffer)[0] ?? Buffer.alloc(0) + filebuffer = getId3TagDataFromFileSync(filebuffer, options)[0] ?? Buffer.alloc(0) } return getTagsFromId3Tag(filebuffer, options) } function readAsync( filebuffer: string | Buffer, - options: Options, + options: FileReadOptions, callback: ReadCallback ) { if (isString(filebuffer)) { - getId3TagDataFromFileAsync(filebuffer).then( - (data) => callback( - null, getTagsFromId3Tag(data[0] ?? Buffer.alloc(0), options) + getId3TagDataFromFileAsync(filebuffer, options).then( + (buffers) => callback( + null, decodeTagBuffers(buffers, options) ), (error) => callback(error, null) ) @@ -70,3 +77,75 @@ function readAsync( callback(null, getTagsFromId3Tag(filebuffer, options)) } } + +// New API + +/** + * Reads ID3-Tags from a buffer. + * + * @public + */ +export function readFromBuffer( + buffer: Buffer, + options: Options = {} +): Tags | TagIdentifiers { + return getTagsFromId3Tag(buffer, options) +} + +/** + * Reads ID3-Tags synchronously from a file. + * + * @public + */ +export function readFromFileSync( + filepath: string, + options: FileReadOptions = {} +): Tags | TagIdentifiers { + const buffers = getId3TagDataFromFileSync(filepath, options) + return decodeTagBuffers(buffers, options) +} + +/** + * Reads ID3-Tags asynchronously from a file. + * + * @public + */ +export function readFromFile( + filepath: string, + callback: ReadCallback +): void + +/** + * Reads ID3-Tags asynchronously from a file. + * + * @public + */ +export function readFromFile( + filepath: string, + options: FileReadOptions, + callback: ReadCallback +): void + +export function readFromFile( + filepath: string, + optionsOrCallback?: FileReadOptions | ReadCallback, + maybeCallback: ReadCallback = () => { /* */ } +): void { + const options: FileReadOptions = + (isFunction(optionsOrCallback) ? {} : optionsOrCallback) ?? {} + const callback = + isFunction(optionsOrCallback) ? optionsOrCallback : maybeCallback + + getId3TagDataFromFileAsync(filepath, options).then( + (buffers) => callback( + null, decodeTagBuffers(buffers, options) + ), + (error) => callback(error, null) + ) +} + +// For now, just take the first one +function decodeTagBuffers(buffers: Buffer[], options: Options) { + const firstBuffer = buffers[0] ?? Buffer.alloc(0) + return getTagsFromId3Tag(firstBuffer, options) +} diff --git a/src/api/update.ts b/src/api/update.ts index e52c7ce..2360a64 100644 --- a/src/api/update.ts +++ b/src/api/update.ts @@ -1,6 +1,6 @@ import { WriteTags } from "../types/Tags" import { Options } from "../types/Options" -import { isFunction, isString, validateString } from "../util" +import { isBuffer, isFunction, isString, validateString } from "../util" import { read } from "./read" import { updateTags } from '../updateTags' import { write } from "./write" @@ -66,7 +66,8 @@ export function update( callback = isFunction(optionsOrCallback) ? optionsOrCallback : callback - const currentTags = read(filebuffer, options) + const currentTags = isBuffer(filebuffer) ? + read(filebuffer, options) : read(filebuffer, options) const updatedTags = updateTags(tags, currentTags) if (isFunction(callback)) { return write(updatedTags, validateString(filebuffer), callback) diff --git a/src/file-read.ts b/src/file-read.ts index fa56923..9f5cae4 100644 --- a/src/file-read.ts +++ b/src/file-read.ts @@ -1,9 +1,13 @@ import * as fs from 'fs' import { Id3TagStreamProcessor } from './file-stream-processor' import { processFileSync, processFileAsync, fsReadAsync } from './util-file' +import { FileOptions } from './types/FileOptions' -export function getId3TagDataFromFileSync(filepath: string) { - const reader = new Id3TagStreamProcessor(12) +export function getId3TagDataFromFileSync( + filepath: string, + options: FileOptions +) { + const reader = new Id3TagStreamProcessor(options.fileBufferSize) processFileSync(filepath, 'r', (fileDescriptor) => { do { const readBuffer = reader.getReadBuffer() @@ -15,8 +19,11 @@ export function getId3TagDataFromFileSync(filepath: string) { return reader.getTags() } -export async function getId3TagDataFromFileAsync(filepath: string) { - const reader = new Id3TagStreamProcessor(12) +export async function getId3TagDataFromFileAsync( + filepath: string, + options: FileOptions +) { + const reader = new Id3TagStreamProcessor(options.fileBufferSize) await processFileAsync(filepath, 'r', async (fileDescriptor) => { do { const readBuffer = reader.getReadBuffer() diff --git a/src/file-stream-processor.ts b/src/file-stream-processor.ts index ebaba9e..8e7fdba 100644 --- a/src/file-stream-processor.ts +++ b/src/file-stream-processor.ts @@ -15,10 +15,8 @@ export class Id3TagStreamProcessor { private rolloverSize = 0 continue = false - constructor(bufferSize: number) { - // TODO enforce min buffer size here, - // i.e. bufferSize + RolloverBufferSize + 1 - this.buffer = Buffer.alloc(bufferSize) + constructor(bufferSize?: number) { + this.buffer = Buffer.alloc(getBufferSize(bufferSize)) } getReadBuffer() { @@ -70,8 +68,6 @@ export class Id3TagRemover { continue = false constructor(bufferSize?: number) { - // TODO enforce min buffer size here, - // i.e. bufferSize + RolloverBufferSize + 1 this.buffer = Buffer.alloc(getBufferSize(bufferSize)) } diff --git a/src/file-write.ts b/src/file-write.ts index eb245f1..cc1b492 100644 --- a/src/file-write.ts +++ b/src/file-write.ts @@ -11,8 +11,8 @@ import { makeTempFilepath } from "./util-file" import * as fs from 'fs' -import { WriteOptions } from "./types/write" import { Id3TagRemover } from "./file-stream-processor" +import { WriteOptions } from "./types/write" export function writeId3TagToFileSync( filepath: string, diff --git a/src/types/FileOptions.ts b/src/types/FileOptions.ts new file mode 100644 index 0000000..58a8218 --- /dev/null +++ b/src/types/FileOptions.ts @@ -0,0 +1,11 @@ +/** + * Options for file operations. + * + * @public + */ +export type FileOptions = { + /** + * File buffer size in bytes. + */ + fileBufferSize?: number +} diff --git a/src/types/read.ts b/src/types/read.ts index 30d7f16..f6aac34 100644 --- a/src/types/read.ts +++ b/src/types/read.ts @@ -1,3 +1,5 @@ +import { FileOptions } from "./FileOptions" +import { Options } from "./Options" import { TagIdentifiers, Tags } from "./Tags" /** @@ -26,3 +28,4 @@ export type ReadErrorCallback = export type ReadCallback = ReadSuccessCallback & ReadErrorCallback +export type FileReadOptions = FileOptions & Options \ No newline at end of file diff --git a/src/types/write.ts b/src/types/write.ts index a989dca..c106c06 100644 --- a/src/types/write.ts +++ b/src/types/write.ts @@ -1,3 +1,5 @@ +import { FileOptions } from "./FileOptions" + /** * Callback signatures for asynchronous update and write operations. * `null` indicated success. @@ -7,14 +9,6 @@ export type WriteCallback = (error: NodeJS.ErrnoException | Error | null) => void -/** - * Options for write operations. - * - * @public - */ -export type WriteOptions = { - /** - * File buffer size in bytes. - */ - fileBufferSize?: number +export type WriteOptions = FileOptions & { + // TODO: add padding options }