From 10f708791237c1e3fc166a404751a0df86b01368 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 2 Jan 2020 16:12:07 +0000 Subject: [PATCH 1/3] Post-decaf cleanup of KeyBuilderTests --- .../filestore/test/unit/js/KeybuilderTests.js | 48 +++++++------------ 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/services/filestore/test/unit/js/KeybuilderTests.js b/services/filestore/test/unit/js/KeybuilderTests.js index 09a0ea8717..5271e892ed 100644 --- a/services/filestore/test/unit/js/KeybuilderTests.js +++ b/services/filestore/test/unit/js/KeybuilderTests.js @@ -1,26 +1,13 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ - -const { assert } = require('chai') -const sinon = require('sinon') -const chai = require('chai') -const should = chai.should() -const { expect } = chai -const modulePath = '../../../app/js/KeyBuilder.js' const SandboxedModule = require('sandboxed-module') +const modulePath = '../../../app/js/KeyBuilder.js' + describe('LocalFileWriter', function() { + let KeyBuilder + const key = 'wombat/potato' + beforeEach(function() { - this.keyBuilder = SandboxedModule.require(modulePath, { + KeyBuilder = SandboxedModule.require(modulePath, { requires: { 'logger-sharelatex': { log() {}, @@ -28,31 +15,28 @@ describe('LocalFileWriter', function() { } } }) - return (this.key = '123/456') }) - return describe('cachedKey', function() { - it('should add the fomat on', function() { + describe('cachedKey', function() { + it('should add the format to the key', function() { const opts = { format: 'png' } - const newKey = this.keyBuilder.addCachingToKey(this.key, opts) - return newKey.should.equal(`${this.key}-converted-cache/format-png`) + const newKey = KeyBuilder.addCachingToKey(key, opts) + newKey.should.equal(`${key}-converted-cache/format-png`) }) - it('should add the style on', function() { + it('should add the style to the key', function() { const opts = { style: 'thumbnail' } - const newKey = this.keyBuilder.addCachingToKey(this.key, opts) - return newKey.should.equal(`${this.key}-converted-cache/style-thumbnail`) + const newKey = KeyBuilder.addCachingToKey(key, opts) + newKey.should.equal(`${key}-converted-cache/style-thumbnail`) }) - return it('should add format on first', function() { + it('should add format first, then style', function() { const opts = { style: 'thumbnail', format: 'png' } - const newKey = this.keyBuilder.addCachingToKey(this.key, opts) - return newKey.should.equal( - `${this.key}-converted-cache/format-png-style-thumbnail` - ) + const newKey = KeyBuilder.addCachingToKey(key, opts) + newKey.should.equal(`${key}-converted-cache/format-png-style-thumbnail`) }) }) }) From dbfacce98822036574945451250b1f71b2537403 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 2 Jan 2020 16:37:47 +0000 Subject: [PATCH 2/3] Post-decaf cleanup of PersistorManager --- services/filestore/app/js/PersistorManager.js | 50 ++--- .../test/unit/js/PersistorManagerTests.js | 185 ++++++------------ 2 files changed, 74 insertions(+), 161 deletions(-) diff --git a/services/filestore/app/js/PersistorManager.js b/services/filestore/app/js/PersistorManager.js index 182e39b085..f8ca7b9d2c 100644 --- a/services/filestore/app/js/PersistorManager.js +++ b/services/filestore/app/js/PersistorManager.js @@ -1,37 +1,19 @@ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -/* - * decaffeinate suggestions: - * DS103: Rewrite code to no longer use __guard__ - * DS205: Consider reworking code to avoid use of IIFEs - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const settings = require('settings-sharelatex') const logger = require('logger-sharelatex') -// assume s3 if none specified -__guard__( - settings != null ? settings.filestore : undefined, - x => x.backend || (settings.filestore.backend = 's3') -) +module.exports = (function() { + logger.log( + { + backend: settings.filestore.backend + }, + 'Loading backend' + ) -logger.log( - { - backend: __guard__( - settings != null ? settings.filestore : undefined, - x1 => x1.backend - ) - }, - 'Loading backend' -) -module.exports = (() => { - switch ( - __guard__( - settings != null ? settings.filestore : undefined, - x2 => x2.backend - ) - ) { + if (!settings.filestore.backend) { + throw new Error('no backend specified - config incomplete') + } + + switch (settings.filestore.backend) { case 'aws-sdk': return require('./AWSSDKPersistorManager') case 's3': @@ -40,13 +22,7 @@ module.exports = (() => { return require('./FSPersistorManager') default: throw new Error( - `Unknown filestore backend: ${settings.filestore.backend}` + `unknown filestore backend: ${settings.filestore.backend}` ) } })() - -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined -} diff --git a/services/filestore/test/unit/js/PersistorManagerTests.js b/services/filestore/test/unit/js/PersistorManagerTests.js index ff49c05ce9..d8fd887265 100644 --- a/services/filestore/test/unit/js/PersistorManagerTests.js +++ b/services/filestore/test/unit/js/PersistorManagerTests.js @@ -1,137 +1,74 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const logger = require('logger-sharelatex') -const { assert } = require('chai') const sinon = require('sinon') const chai = require('chai') -const should = chai.should() const { expect } = chai -const modulePath = '../../../app/js/PersistorManager.js' const SandboxedModule = require('sandboxed-module') -describe('PersistorManagerTests', function() { +const modulePath = '../../../app/js/PersistorManager.js' + +describe('PersistorManager', function() { + let PersistorManager, + FSPersistorManager, + S3PersistorManager, + settings, + requires + beforeEach(function() { - return (this.S3PersistorManager = { - getFileStream: sinon.stub(), - checkIfFileExists: sinon.stub(), - deleteFile: sinon.stub(), - deleteDirectory: sinon.stub(), - sendStream: sinon.stub(), - insertFile: sinon.stub() - }) + FSPersistorManager = { + wrappedMethod: sinon.stub().returns('FSPersistorManager') + } + S3PersistorManager = { + wrappedMethod: sinon.stub().returns('S3PersistorManager') + } + + settings = { + filestore: {} + } + + requires = { + './S3PersistorManager': S3PersistorManager, + './FSPersistorManager': FSPersistorManager, + 'settings-sharelatex': settings, + 'logger-sharelatex': { + log() {}, + err() {} + } + } }) - describe('test s3 mixin', function() { - beforeEach(function() { - this.settings = { - filestore: { - backend: 's3' - } - } - this.requires = { - './S3PersistorManager': this.S3PersistorManager, - 'settings-sharelatex': this.settings, - 'logger-sharelatex': { - log() {}, - err() {} - } - } - return (this.PersistorManager = SandboxedModule.require(modulePath, { - requires: this.requires - })) - }) + it('should implement the S3 wrapped method when S3 is configured', function() { + settings.filestore.backend = 's3' + PersistorManager = SandboxedModule.require(modulePath, { requires }) - it('should load getFileStream', function(done) { - this.PersistorManager.should.respondTo('getFileStream') - this.PersistorManager.getFileStream() - this.S3PersistorManager.getFileStream.calledOnce.should.equal(true) - return done() - }) - - it('should load checkIfFileExists', function(done) { - this.PersistorManager.checkIfFileExists() - this.S3PersistorManager.checkIfFileExists.calledOnce.should.equal(true) - return done() - }) - - it('should load deleteFile', function(done) { - this.PersistorManager.deleteFile() - this.S3PersistorManager.deleteFile.calledOnce.should.equal(true) - return done() - }) - - it('should load deleteDirectory', function(done) { - this.PersistorManager.deleteDirectory() - this.S3PersistorManager.deleteDirectory.calledOnce.should.equal(true) - return done() - }) - - it('should load sendStream', function(done) { - this.PersistorManager.sendStream() - this.S3PersistorManager.sendStream.calledOnce.should.equal(true) - return done() - }) - - return it('should load insertFile', function(done) { - this.PersistorManager.insertFile() - this.S3PersistorManager.insertFile.calledOnce.should.equal(true) - return done() - }) + expect(PersistorManager).to.respondTo('wrappedMethod') + expect(PersistorManager.wrappedMethod()).to.equal('S3PersistorManager') }) - describe('test unspecified mixins', () => - it('should load s3 when no wrapper specified', function(done) { - this.settings = { filestore: {} } - this.requires = { - './S3PersistorManager': this.S3PersistorManager, - 'settings-sharelatex': this.settings, - 'logger-sharelatex': { - log() {}, - err() {} - } - } - this.PersistorManager = SandboxedModule.require(modulePath, { - requires: this.requires - }) - this.PersistorManager.should.respondTo('getFileStream') - this.PersistorManager.getFileStream() - this.S3PersistorManager.getFileStream.calledOnce.should.equal(true) - return done() - })) + it('should implement the FS wrapped method when FS is configured', function() { + settings.filestore.backend = 'fs' + PersistorManager = SandboxedModule.require(modulePath, { requires }) - return describe('test invalid mixins', () => - it('should not load an invalid wrapper', function(done) { - this.settings = { - filestore: { - backend: 'magic' - } - } - this.requires = { - './S3PersistorManager': this.S3PersistorManager, - 'settings-sharelatex': this.settings, - 'logger-sharelatex': { - log() {}, - err() {} - } - } - this.fsWrapper = null - try { - this.PersistorManager = SandboxedModule.require(modulePath, { - requires: this.requires - }) - } catch (error) { - assert.equal('Unknown filestore backend: magic', error.message) - } - assert.isNull(this.fsWrapper) - return done() - })) + expect(PersistorManager).to.respondTo('wrappedMethod') + expect(PersistorManager.wrappedMethod()).to.equal('FSPersistorManager') + }) + + it('should throw an error when the backend is not configured', function() { + try { + SandboxedModule.require(modulePath, { requires }) + } catch (err) { + expect(err.message).to.equal('no backend specified - config incomplete') + return + } + expect('should have caught an error').not.to.exist + }) + + it('should throw an error when the backend is unknown', function() { + settings.filestore.backend = 'magic' + try { + SandboxedModule.require(modulePath, { requires }) + } catch (err) { + expect(err.message).to.equal('unknown filestore backend: magic') + return + } + expect('should have caught an error').not.to.exist + }) }) From 0329c759dc99f836e511e9c38c4a1e1410385e22 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 4 Jan 2020 17:27:12 +0000 Subject: [PATCH 3/3] Remove wrapper function in PersistorManager --- services/filestore/app/js/PersistorManager.js | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/services/filestore/app/js/PersistorManager.js b/services/filestore/app/js/PersistorManager.js index f8ca7b9d2c..8124d66101 100644 --- a/services/filestore/app/js/PersistorManager.js +++ b/services/filestore/app/js/PersistorManager.js @@ -1,28 +1,26 @@ const settings = require('settings-sharelatex') const logger = require('logger-sharelatex') -module.exports = (function() { - logger.log( - { - backend: settings.filestore.backend - }, - 'Loading backend' - ) +logger.log( + { + backend: settings.filestore.backend + }, + 'Loading backend' +) +if (!settings.filestore.backend) { + throw new Error('no backend specified - config incomplete') +} - if (!settings.filestore.backend) { - throw new Error('no backend specified - config incomplete') - } - - switch (settings.filestore.backend) { - case 'aws-sdk': - return require('./AWSSDKPersistorManager') - case 's3': - return require('./S3PersistorManager') - case 'fs': - return require('./FSPersistorManager') - default: - throw new Error( - `unknown filestore backend: ${settings.filestore.backend}` - ) - } -})() +switch (settings.filestore.backend) { + case 'aws-sdk': + module.exports = require('./AWSSDKPersistorManager') + break + case 's3': + module.exports = require('./S3PersistorManager') + break + case 'fs': + module.exports = require('./FSPersistorManager') + break + default: + throw new Error(`unknown filestore backend: ${settings.filestore.backend}`) +}