From 5790e42ed64dd3e706edb579d8ccfc6b320f7457 Mon Sep 17 00:00:00 2001 From: Zazama Date: Fri, 7 Apr 2023 22:06:09 +0200 Subject: [PATCH 1/8] Remove unreachable statement --- src/api/remove.ts | 9 +-------- src/id3-tag.ts | 4 ---- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/api/remove.ts b/src/api/remove.ts index 5ef74e9..d468e80 100644 --- a/src/api/remove.ts +++ b/src/api/remove.ts @@ -26,7 +26,7 @@ export type RemoveCallback = * * @public */ -export function removeTags(filepath: string): boolean | Error +export function removeTags(filepath: string): true | Error /** * Removes asynchronously any written ID3-Frames from the specified file. @@ -51,9 +51,6 @@ function removeTagsSync(filepath: string) { } const newData = removeTagsFromBuffer(data) - if(!newData) { - return false - } try { fs.writeFileSync(filepath, newData, 'binary') @@ -72,10 +69,6 @@ function removeTagsAsync(filepath: string, callback: RemoveCallback) { } const newData = removeTagsFromBuffer(data) - if(!newData) { - callback(error) - return - } fs.writeFile(filepath, newData, 'binary', (error) => { if(error) { diff --git a/src/id3-tag.ts b/src/id3-tag.ts index 98f0b99..c025ce3 100644 --- a/src/id3-tag.ts +++ b/src/id3-tag.ts @@ -92,10 +92,6 @@ export function removeId3Tag(data: Buffer) { } 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([ From 920dea15d5d2ee0049f3a10b02422a381e3ac3d8 Mon Sep 17 00:00:00 2001 From: Zazama Date: Fri, 7 Apr 2023 22:11:46 +0200 Subject: [PATCH 2/8] Add removeTags tests --- test/api/remove.ts | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 test/api/remove.ts diff --git a/test/api/remove.ts b/test/api/remove.ts new file mode 100644 index 0000000..d6750ff --- /dev/null +++ b/test/api/remove.ts @@ -0,0 +1,70 @@ +import * as NodeID3 from '../../index' +import assert = require('assert') +import chai = require('chai') +import * as fs from 'fs' + +describe('NodeID3 API', function () { + describe('#removeTags()', function() { + const nonExistingFilepath = './hopefully-does-not-exist.mp3' + it('sync not existing filepath', function() { + chai.assert.isFalse(fs.existsSync(nonExistingFilepath)) + chai.assert.instanceOf( + NodeID3.removeTags(nonExistingFilepath), Error + ) + }) + it('async not existing filepath', function() { + chai.assert.isFalse(fs.existsSync(nonExistingFilepath)) + NodeID3.removeTags(nonExistingFilepath, function(err) { + if(!(err instanceof Error)) { + assert.fail("No error thrown on non-existing filepath") + } + }) + }) + + const titleTag = { + title: "abc" + } satisfies NodeID3.WriteTags + const filepath = './testfile.mp3' + + describe('valid buffer', function() { + const prefixBuffer = Buffer.from([0x01, 0x02, 0x03]) + const postfixBuffer = Buffer.from([0x04, 0x05, 0x06]) + const buffer = Buffer.concat([ + prefixBuffer, + NodeID3.create(titleTag), + postfixBuffer + ]) + const bufferAfterRemove = Buffer.concat([ + prefixBuffer, + postfixBuffer + ]) + + beforeEach(function() { + fs.writeFileSync(filepath, buffer) + }) + + afterEach(function() { + fs.unlinkSync(filepath) + }) + + it('sync remove tags from file', function() { + NodeID3.removeTags(filepath) + assert.deepStrictEqual( + fs.readFileSync(filepath), + bufferAfterRemove + ) + }) + + it('async remove tags from file', function(done) { + NodeID3.removeTags(filepath, (err) => { + assert.equal(err, null) + assert.deepStrictEqual( + fs.readFileSync(filepath), + bufferAfterRemove + ) + done() + }) + }) + }) + }) +}) From 06d498bed0d391ff0139e38a1c078f09d1562f56 Mon Sep 17 00:00:00 2001 From: Zazama Date: Fri, 7 Apr 2023 22:47:31 +0200 Subject: [PATCH 3/8] Complete update api test coverage --- src/api/update.ts | 22 +++++++++++----------- test/api/update.ts | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 test/api/update.ts diff --git a/src/api/update.ts b/src/api/update.ts index 0c67a77..cf84eaf 100644 --- a/src/api/update.ts +++ b/src/api/update.ts @@ -5,6 +5,17 @@ import { read } from "./read" import { updateTags } from '../updateTags' import { write, WriteCallback } from "./write" +/** + * Updates ID3-Tags asynchronously in the specified file. + * + * @public + */ +export function update( + tags: WriteTags, + filebuffer: string | Buffer, + callback: WriteCallback +): void + /** * Updates ID3-Tags from the given buffer. * @@ -27,17 +38,6 @@ import { write, WriteCallback } from "./write" options?: Options ): true | Error -/** - * Updates ID3-Tags asynchronously in the specified file. - * - * @public - */ - export function update( - tags: WriteTags, - filebuffer: string | Buffer, - callback: WriteCallback -): void - /** * Updates ID3-Tags asynchronously from the given buffer or specified file. * diff --git a/test/api/update.ts b/test/api/update.ts new file mode 100644 index 0000000..3b1586f --- /dev/null +++ b/test/api/update.ts @@ -0,0 +1,45 @@ +import * as NodeID3 from '../../index' +import assert = require('assert') +import chai = require('chai') +import * as fs from 'fs' +import { WriteCallback } from '../../index' + +describe('NodeID3 API', function () { + describe('#update()', function() { + const titleTag = { + title: "abc" + } satisfies NodeID3.WriteTags + const albumTag = { + album: "def" + } + const tags = {...titleTag, ...albumTag} + const filepath = './testfile.mp3' + + beforeEach(function() { + fs.writeFileSync(filepath, NodeID3.create(titleTag)) + }) + + it('sync add tag to existing', function() { + chai.assert.isTrue(NodeID3.update(albumTag, filepath)) + assert.deepStrictEqual( + NodeID3.read(filepath, {noRaw: true}), + tags + ) + }) + + it('async add tag to existing', function(done) { + NodeID3.update(albumTag, filepath, (error, data) => { + chai.assert.isNull(error) + assert.deepStrictEqual( + NodeID3.read(filepath, {noRaw: true}), + tags + ) + done() + }) + }) + + afterEach(function() { + fs.unlinkSync(filepath) + }) + }) +}) \ No newline at end of file From 109374c9b3114ed5f781cf2f6a724233532bff18 Mon Sep 17 00:00:00 2001 From: Zazama Date: Fri, 7 Apr 2023 23:43:24 +0200 Subject: [PATCH 4/8] Improve frames test coverage --- test/frames.ts | 122 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 1 deletion(-) diff --git a/test/frames.ts b/test/frames.ts index 8af1649..3f92acb 100644 --- a/test/frames.ts +++ b/test/frames.ts @@ -1,14 +1,15 @@ import * as NodeID3 from '../index' import assert = require('assert') +import { expect } from 'chai' /** * Some characters to test unicode encoding. */ const unicodeTestCharacters = "-äé" +const TagConstants = NodeID3.TagConstants describe('NodeID3 frames', function () { it('read() matches create()', function () { - const TagConstants = NodeID3.TagConstants const tags = { /** * COMM @@ -162,4 +163,123 @@ describe('NodeID3 frames', function () { const readTags = NodeID3.read(createdBuffer, { noRaw: true}) assert.deepStrictEqual(tags, readTags) }) + + describe('read() does not match create()', function() { + it('create throws', function() { + const throwingTags = [ + { POPM: {} }, + { POPM: { + email: 'test' + }}, + { POPM: { + email: 'test', + rating: 1 + }}, + { CTOC: {} }, + { USLT: {} }, + { CHAP: {} }, + { COMM: {} }, + { TALB: null }, + { WCOM: null }, + { APIC: { + mime: "a", + type: { + id: TagConstants.AttachedPicture.PictureType.FRONT_COVER + }, + description: "d", + imageBuffer: "" + }}, + { COMM: { + language: 'asdf', + text: 'text' + }}, + { COMR: { + prices: { + EURO: 13 + } + }} + ] + + for(const throwingTag of throwingTags) { + expect(() => NodeID3.create(throwingTag as never)).to.throw() + } + }) + + it('frame builder changes data', function() { + const tags = { + unsynchronisedLyrics: 'just a string', + commercialFrame: { + validUntil: { year: 2023, month: 9, day: 'a'}, + receivedAs: TagConstants.CommercialFrame.ReceivedAs.OTHER, + }, + tableOfContents: { + elementID: "1" + }, + synchronisedLyrics: { + language: "eng", + timeStampFormat: TagConstants.TimeStampFormat.MILLISECONDS, + contentType: TagConstants.SynchronisedLyrics.ContentType.LYRICS, + synchronisedText: [] + }, + private: { + data: 'string' + }, + uniqueFileIdentifier: { + ownerIdentifier: 'a', + identifier: 'b' + }, + image: Buffer.from([0xff, 0xd8, 0xff, 0x00]) + } + const expectedTags = { + unsynchronisedLyrics: { + language: 'eng', + shortText: '', + text: tags.unsynchronisedLyrics + }, + commercialFrame: [{ + ...tags.commercialFrame, + validUntil: { + year: 0, month: 0, day: 0 + }, + prices: {}, + contactUrl: '', + nameOfSeller: '', + description: '', + }], + tableOfContents: [{ + ...tags.tableOfContents, + isOrdered: false, + elements: [], + tags: { raw: {} } + }], + synchronisedLyrics: [{ + ...tags.synchronisedLyrics, + shortText: '' + }], + private: [{ + ownerIdentifier: '', + data: Buffer.from(tags.private.data, 'utf8') + }], + uniqueFileIdentifier: [{ + ...tags.uniqueFileIdentifier, + identifier: Buffer.from( + tags.uniqueFileIdentifier.identifier + , 'utf8') + }], + image: { + mime: "image/jpeg", + type: { + id: TagConstants.AttachedPicture.PictureType.FRONT_COVER, + name: "front cover" + }, + description: '', + imageBuffer: tags.image + } + } satisfies NodeID3.Tags + assert.deepStrictEqual( + NodeID3.read(NodeID3.create(tags as never), {noRaw: true}), + expectedTags + ) + }) + }) }) From 8db01a29ed2c3b091308408a3962b0086e96f0c7 Mon Sep 17 00:00:00 2001 From: Zazama Date: Sat, 8 Apr 2023 00:01:49 +0200 Subject: [PATCH 5/8] Test for update compare key --- test/api/update.ts | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/api/update.ts b/test/api/update.ts index 3b1586f..1f857ea 100644 --- a/test/api/update.ts +++ b/test/api/update.ts @@ -38,6 +38,40 @@ describe('NodeID3 API', function () { }) }) + it('compare key', function() { + const beforeTags = { + userDefinedText: [{ + description: 'description', + value: 'some value' + }], + private: [{ + ownerIdentifier: 'ownerIdentifier', + data: Buffer.from('data') + }] + } satisfies NodeID3.Tags + const addTags = { + userDefinedText: [{ + description: 'description', + value: 'some other value' + }], + private: { + ownerIdentifier: 'ownerIdentifier', + data: Buffer.from('data2') + } + } satisfies NodeID3.Tags + const afterTags = { + userDefinedText: addTags.userDefinedText, + private: [...beforeTags.private, addTags.private] + } satisfies NodeID3.Tags + const beforeBuffer = NodeID3.create(beforeTags) + const afterBuffer = NodeID3.update(addTags, beforeBuffer) + + assert.deepStrictEqual( + NodeID3.read(afterBuffer, {noRaw: true}), + afterTags + ) + }) + afterEach(function() { fs.unlinkSync(filepath) }) From fada4ce885293d82b509d0497e3e2842f4ee50ea Mon Sep 17 00:00:00 2001 From: Zazama Date: Sat, 8 Apr 2023 13:23:46 +0200 Subject: [PATCH 6/8] Add eol newline rule to eslint --- .eslintrc.json | 5 +++-- src/definitions/Encoding.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 5a79582..955a085 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -12,7 +12,7 @@ "parserOptions": { "sourceType": "module" }, - "ignorePatterns": [ "/dist/**" ], + "ignorePatterns": [ "/dist/**", "/example/**" ], "overrides": [ { "files": ["**/*.ts"], @@ -50,6 +50,7 @@ "semi": [ "error", "never" ], "@typescript-eslint/no-unused-vars": [ "off", { "argsIgnorePattern": "^_" } - ] + ], + "eol-last": [ "error", "always" ] } } diff --git a/src/definitions/Encoding.ts b/src/definitions/Encoding.ts index 805cc23..1d7bf42 100644 --- a/src/definitions/Encoding.ts +++ b/src/definitions/Encoding.ts @@ -20,4 +20,4 @@ export const TextEncoding = { * Terminated with one zero byte. */ UTF_8: 3 -} as const \ No newline at end of file +} as const From abb577bef401a99c108a929836ccc156450407e4 Mon Sep 17 00:00:00 2001 From: Zazama Date: Sat, 8 Apr 2023 13:24:42 +0200 Subject: [PATCH 7/8] Explain update compare key tests further --- test/api/update.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/api/update.ts b/test/api/update.ts index 1f857ea..42ecdc1 100644 --- a/test/api/update.ts +++ b/test/api/update.ts @@ -2,15 +2,14 @@ import * as NodeID3 from '../../index' import assert = require('assert') import chai = require('chai') import * as fs from 'fs' -import { WriteCallback } from '../../index' describe('NodeID3 API', function () { describe('#update()', function() { const titleTag = { - title: "abc" + title: 'title' } satisfies NodeID3.WriteTags const albumTag = { - album: "def" + album: 'album' } const tags = {...titleTag, ...albumTag} const filepath = './testfile.mp3' @@ -28,7 +27,7 @@ describe('NodeID3 API', function () { }) it('async add tag to existing', function(done) { - NodeID3.update(albumTag, filepath, (error, data) => { + NodeID3.update(albumTag, filepath, (error) => { chai.assert.isNull(error) assert.deepStrictEqual( NodeID3.read(filepath, {noRaw: true}), @@ -38,7 +37,8 @@ describe('NodeID3 API', function () { }) }) - it('compare key', function() { + // TODO: Remove in new API release + it('update compare key is respected when available', function() { const beforeTags = { userDefinedText: [{ description: 'description', @@ -59,6 +59,10 @@ describe('NodeID3 API', function () { data: Buffer.from('data2') } } satisfies NodeID3.Tags + + // userDefinedText should update the value because of equal descriptions. + // private frame does not have an update compare key specified, + // which is why the new one is added next to the old. const afterTags = { userDefinedText: addTags.userDefinedText, private: [...beforeTags.private, addTags.private] @@ -76,4 +80,4 @@ describe('NodeID3 API', function () { fs.unlinkSync(filepath) }) }) -}) \ No newline at end of file +}) From 3af779d7b66462759b00a24cd269acbf65f7c705 Mon Sep 17 00:00:00 2001 From: Zazama Date: Sat, 8 Apr 2023 13:24:58 +0200 Subject: [PATCH 8/8] Rename variables to better match the tags --- test/api/remove.ts | 4 ++-- test/frames.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/api/remove.ts b/test/api/remove.ts index d6750ff..b503193 100644 --- a/test/api/remove.ts +++ b/test/api/remove.ts @@ -16,13 +16,13 @@ describe('NodeID3 API', function () { chai.assert.isFalse(fs.existsSync(nonExistingFilepath)) NodeID3.removeTags(nonExistingFilepath, function(err) { if(!(err instanceof Error)) { - assert.fail("No error thrown on non-existing filepath") + assert.fail('No error thrown on non-existing filepath') } }) }) const titleTag = { - title: "abc" + title: 'title' } satisfies NodeID3.WriteTags const filepath = './testfile.mp3' diff --git a/test/frames.ts b/test/frames.ts index 3f92acb..8d7f28d 100644 --- a/test/frames.ts +++ b/test/frames.ts @@ -200,9 +200,9 @@ describe('NodeID3 frames', function () { }} ] - for(const throwingTag of throwingTags) { + throwingTags.forEach((throwingTag) => { expect(() => NodeID3.create(throwingTag as never)).to.throw() - } + }) }) it('frame builder changes data', function() {