From 9a6e954e1fe7653d41993f05d12e6176e48d2784 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 Jan 2022 10:41:31 +0000 Subject: [PATCH] Merge pull request #6234 from overleaf/jpa-web-owns-spelling-preferences [misc] move ownership of spellingPreferences collection to web GitOrigin-RevId: f2584a1119a578c3df15371c6798923a4f2d15ae --- services/spelling/app.js | 25 +-- services/spelling/app/js/MongoCache.js | 11 -- .../spelling/app/js/SpellingAPIController.js | 57 ------ .../spelling/app/js/SpellingAPIManager.js | 59 +----- services/spelling/config/settings.defaults.js | 10 -- services/spelling/test/acceptance/js/Init.js | 3 - .../spelling/test/acceptance/js/LearnTest.js | 103 ----------- .../test/unit/js/SpellingAPIManagerTests.js | 168 +----------------- .../src/Features/Project/ProjectController.js | 2 +- .../Features/Spelling}/LearnedWordsManager.js | 34 +--- .../Features/Spelling/SpellingController.js | 10 ++ .../src/Features/Spelling/SpellingHandler.js | 105 +---------- services/web/app/src/router.js | 7 +- services/web/test/acceptance/src/LearnTest.js | 52 ++++++ .../src/Project/ProjectControllerTests.js | 2 +- .../src/Spelling}/LearnedWordsManagerTests.js | 45 +---- .../src/Spelling/SpellingControllerTests.js | 1 + .../unit/src/Spelling/SpellingHandlerTests.js | 124 ------------- 18 files changed, 92 insertions(+), 726 deletions(-) delete mode 100644 services/spelling/app/js/MongoCache.js delete mode 100644 services/spelling/test/acceptance/js/LearnTest.js rename services/{spelling/app/js => web/app/src/Features/Spelling}/LearnedWordsManager.js (67%) create mode 100644 services/web/test/acceptance/src/LearnTest.js rename services/{spelling/test/unit/js => web/test/unit/src/Spelling}/LearnedWordsManagerTests.js (68%) delete mode 100644 services/web/test/unit/src/Spelling/SpellingHandlerTests.js diff --git a/services/spelling/app.js b/services/spelling/app.js index 3f803fccc0..8dbcd2a30f 100644 --- a/services/spelling/app.js +++ b/services/spelling/app.js @@ -16,7 +16,6 @@ if ((Settings.sentry != null ? Settings.sentry.dsn : undefined) != null) { } metrics.memory.monitor(logger) -const mongodb = require('./app/js/mongodb') const SpellingAPIController = require('./app/js/SpellingAPIController') const express = require('express') const app = express() @@ -27,11 +26,7 @@ const HealthCheckController = require('./app/js/HealthCheckController') app.use(bodyParser.json({ limit: '2mb' })) app.use(metrics.http.monitor(logger)) -app.delete('/user/:user_id', SpellingAPIController.deleteDic) -app.get('/user/:user_id', SpellingAPIController.getDic) app.post('/user/:user_id/check', SpellingAPIController.check) -app.post('/user/:user_id/learn', SpellingAPIController.learn) -app.post('/user/:user_id/unlearn', SpellingAPIController.unlearn) app.get('/status', (req, res) => res.send({ status: 'spelling api is up' })) app.get('/health_check', HealthCheckController.healthCheck) @@ -45,20 +40,12 @@ const port = settings && settings.port ? settings.port : 3005 if (!module.parent) { // application entry point, called directly - mongodb - .waitForDb() - .then(() => { - app.listen(port, host, function (error) { - if (error != null) { - throw error - } - return logger.info(`spelling starting up, listening on ${host}:${port}`) - }) - }) - .catch(err => { - logger.fatal({ err }, 'Cannot connect to mongo. Exiting.') - process.exit(1) - }) + app.listen(port, host, function (error) { + if (error != null) { + throw error + } + return logger.info(`spelling starting up, listening on ${host}:${port}`) + }) } module.exports = app diff --git a/services/spelling/app/js/MongoCache.js b/services/spelling/app/js/MongoCache.js deleted file mode 100644 index b78b27f5a5..0000000000 --- a/services/spelling/app/js/MongoCache.js +++ /dev/null @@ -1,11 +0,0 @@ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -const LRU = require('lru-cache') -const cacheOpts = { - max: 15000, - maxAge: 1000 * 60 * 60 * 10, -} - -const cache = new LRU(cacheOpts) - -module.exports = cache diff --git a/services/spelling/app/js/SpellingAPIController.js b/services/spelling/app/js/SpellingAPIController.js index dc0bd241f4..77d8f52105 100644 --- a/services/spelling/app/js/SpellingAPIController.js +++ b/services/spelling/app/js/SpellingAPIController.js @@ -10,12 +10,6 @@ function extractCheckRequestData(req) { return { token, wordCount } } -function extractLearnRequestData(req) { - const token = req.params ? req.params.user_id : undefined - const word = req.body ? req.body.word : undefined - return { token, word } -} - module.exports = { check(req, res) { metrics.inc('spelling-check', 0.1) @@ -34,55 +28,4 @@ module.exports = { res.send(result) }) }, - - learn(req, res, next) { - metrics.inc('spelling-learn', 0.1) - const { token, word } = extractLearnRequestData(req) - logger.info({ token, word }, 'learning word') - SpellingAPIManager.learnWord(token, req.body, function (error) { - if (error != null) { - return next(OError.tag(error)) - } - res.sendStatus(204) - }) - }, - - unlearn(req, res, next) { - metrics.inc('spelling-unlearn', 0.1) - const { token, word } = extractLearnRequestData(req) - logger.info({ token, word }, 'unlearning word') - SpellingAPIManager.unlearnWord(token, req.body, function (error) { - if (error != null) { - return next(OError.tag(error)) - } - res.sendStatus(204) - }) - }, - - deleteDic(req, res, next) { - const { token, word } = extractLearnRequestData(req) - logger.log({ token, word }, 'deleting user dictionary') - SpellingAPIManager.deleteDic(token, function (error) { - if (error != null) { - return next(OError.tag(error)) - } - res.sendStatus(204) - }) - }, - - getDic(req, res, next) { - const token = req.params ? req.params.user_id : undefined - logger.info( - { - token, - }, - 'getting user dictionary' - ) - SpellingAPIManager.getDic(token, function (error, words) { - if (error != null) { - return next(OError.tag(error)) - } - res.send(words) - }) - }, } diff --git a/services/spelling/app/js/SpellingAPIManager.js b/services/spelling/app/js/SpellingAPIManager.js index 9232525581..336037a880 100644 --- a/services/spelling/app/js/SpellingAPIManager.js +++ b/services/spelling/app/js/SpellingAPIManager.js @@ -7,53 +7,13 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const ASpell = require('./ASpell') -const LearnedWordsManager = require('./LearnedWordsManager') const { callbackify } = require('util') const OError = require('@overleaf/o-error') -const Settings = require('@overleaf/settings') // The max number of words checked in a single request const REQUEST_LIMIT = 10000 -const SpellingAPIManager = { - whitelist: Settings.ignoredMisspellings, - - learnWord(token, request, callback) { - if (callback == null) { - callback = () => {} - } - if (request.word == null) { - return callback(new OError('malformed JSON')) - } - if (token == null) { - return callback(new OError('no token provided')) - } - - return LearnedWordsManager.learnWord(token, request.word, callback) - }, - - unlearnWord(token, request, callback) { - if (callback == null) { - callback = () => {} - } - if (request.word == null) { - return callback(new OError('malformed JSON')) - } - if (token == null) { - return callback(new OError('no token provided')) - } - - return LearnedWordsManager.unlearnWord(token, request.word, callback) - }, - - deleteDic(token, callback) { - return LearnedWordsManager.deleteUsersLearnedWords(token, callback) - }, - - getDic(token, callback) { - return LearnedWordsManager.getLearnedWordsNoCache(token, callback) - }, -} +const SpellingAPIManager = {} const promises = { async runRequest(token, request) { @@ -66,22 +26,7 @@ const promises = { const wordSlice = request.words.slice(0, REQUEST_LIMIT) const misspellings = await ASpell.promises.checkWords(lang, wordSlice) - - if (token && !request.skipLearnedWords) { - const learnedWords = await LearnedWordsManager.promises.getLearnedWords( - token - ) - const notLearntMisspellings = misspellings.filter(m => { - const word = wordSlice[m.index] - return ( - learnedWords.indexOf(word) === -1 && - SpellingAPIManager.whitelist.indexOf(word) === -1 - ) - }) - return { misspellings: notLearntMisspellings } - } else { - return { misspellings } - } + return { misspellings } }, } diff --git a/services/spelling/config/settings.defaults.js b/services/spelling/config/settings.defaults.js index e0e7efdfe3..64e039c5cd 100644 --- a/services/spelling/config/settings.defaults.js +++ b/services/spelling/config/settings.defaults.js @@ -8,16 +8,6 @@ module.exports = { }, }, - mongo: { - options: { - useUnifiedTopology: - (process.env.MONGO_USE_UNIFIED_TOPOLOGY || 'true') === 'true', - }, - url: - process.env.MONGO_CONNECTION_STRING || - `mongodb://${process.env.MONGO_HOST || 'localhost'}/sharelatex`, - }, - cacheDir: Path.resolve('cache'), healthCheckUserId: '53c64d2fd68c8d000010bb5f', diff --git a/services/spelling/test/acceptance/js/Init.js b/services/spelling/test/acceptance/js/Init.js index 45732a30b8..33f454e6b5 100644 --- a/services/spelling/test/acceptance/js/Init.js +++ b/services/spelling/test/acceptance/js/Init.js @@ -1,8 +1,5 @@ -const { waitForDb } = require('../../../app/js/mongodb') const App = require('../../../app.js') const { PORT } = require('./helpers/request') - -before(waitForDb) before(function (done) { return App.listen(PORT, 'localhost', done) }) diff --git a/services/spelling/test/acceptance/js/LearnTest.js b/services/spelling/test/acceptance/js/LearnTest.js deleted file mode 100644 index 231b1fcd60..0000000000 --- a/services/spelling/test/acceptance/js/LearnTest.js +++ /dev/null @@ -1,103 +0,0 @@ -const { expect } = require('chai') -const request = require('./helpers/request') - -const USER_ID = 101 - -const checkWord = words => - request.post({ - url: `/user/${USER_ID}/check`, - body: JSON.stringify({ - words, - }), - }) - -const learnWord = word => - request.post({ - url: `/user/${USER_ID}/learn`, - body: JSON.stringify({ - word, - }), - }) - -const unlearnWord = word => - request.post({ - url: `/user/${USER_ID}/unlearn`, - body: JSON.stringify({ - word, - }), - }) - -const getDict = () => - request.get({ - url: `/user/${USER_ID}`, - }) - -const deleteDict = () => - request.del({ - url: `/user/${USER_ID}`, - }) - -describe('learning words', function () { - afterEach(async function () { - await deleteDict() - }) - - it('should return status 204 when posting a word successfully', async function () { - const response = await learnWord('abcd') - expect(response.statusCode).to.equal(204) - }) - - it('should not learn the same word twice', async function () { - await learnWord('foobar') - const learnResponse = await learnWord('foobar') - expect(learnResponse.statusCode).to.equal(204) - - const dictResponse = await getDict() - const responseBody = JSON.parse(dictResponse.body) - // the response from getlearnedwords filters out duplicates, so this test - // can succeed even if the word is stored twice in the database - expect(responseBody.length).to.equals(1) - }) - - it('should return no misspellings after a word is learnt', async function () { - const response = await checkWord(['abv']) - const responseBody = JSON.parse(response.body) - expect(responseBody.misspellings.length).to.equals(1) - - await learnWord('abv') - - const response2 = await checkWord(['abv']) - const responseBody2 = JSON.parse(response2.body) - expect(responseBody2.misspellings.length).to.equals(0) - }) - - it('should return misspellings again after a personal dictionary is deleted', async function () { - await learnWord('bvc') - await deleteDict() - - const response = await checkWord(['bvc']) - const responseBody = JSON.parse(response.body) - expect(responseBody.misspellings.length).to.equals(1) - }) -}) - -describe('unlearning words', function () { - it('should return status 204 when posting a word successfully', async function () { - const response = await unlearnWord('anything') - expect(response.statusCode).to.equal(204) - }) - - it('should return misspellings after a word is unlearnt', async function () { - await learnWord('abv') - - const response = await checkWord(['abv']) - const responseBody = JSON.parse(response.body) - expect(responseBody.misspellings.length).to.equals(0) - - await unlearnWord('abv') - - const response2 = await checkWord(['abv']) - const responseBody2 = JSON.parse(response2.body) - expect(responseBody2.misspellings.length).to.equals(1) - }) -}) diff --git a/services/spelling/test/unit/js/SpellingAPIManagerTests.js b/services/spelling/test/unit/js/SpellingAPIManagerTests.js index bf168bfdb5..47d731c08e 100644 --- a/services/spelling/test/unit/js/SpellingAPIManagerTests.js +++ b/services/spelling/test/unit/js/SpellingAPIManagerTests.js @@ -15,42 +15,22 @@ describe('SpellingAPIManager', function () { beforeEach(function () { this.token = 'user-id-123' this.ASpell = {} - this.learnedWords = ['lerned'] - this.LearnedWordsManager = { - getLearnedWords: sinon.stub().callsArgWith(1, null, this.learnedWords), - learnWord: sinon.stub().callsArg(2), - unlearnWord: sinon.stub().callsArg(2), - promises: { - getLearnedWords: sinon.stub().returns(promiseStub(this.learnedWords)), - }, - } - this.SpellingAPIManager = SandboxedModule.require(modulePath, { requires: { './ASpell': this.ASpell, '@overleaf/settings': { ignoredMisspellings: ['ShareLaTeX'] }, - './LearnedWordsManager': this.LearnedWordsManager, }, }) }) describe('runRequest', function () { beforeEach(function () { - this.nonLearnedWords = [ - 'some', - 'words', - 'htat', - 'are', - 'speled', - 'rong', - 'lerned', - ] - this.allWords = this.nonLearnedWords.concat(this.learnedWords) + this.nonLearnedWords = ['some', 'words', 'htat', 'are', 'speled', 'rong'] + this.allWords = this.nonLearnedWords this.misspellings = [ { index: 2, suggestions: ['that'] }, { index: 4, suggestions: ['spelled'] }, { index: 5, suggestions: ['wrong', 'ring'] }, - { index: 6, suggestions: ['learned'] }, ] this.misspellingsWithoutLearnedWords = this.misspellings.slice(0, 3) @@ -99,24 +79,6 @@ describe('SpellingAPIManager', function () { }) }) - describe('with a missing token', function () { - beforeEach(function (done) { - this.SpellingAPIManager.runRequest( - null, - { words: this.allWords }, - (error, result) => { - this.error = error - this.result = result - done() - } - ) - }) - - it('should spell check without using any learned words', function () { - this.LearnedWordsManager.getLearnedWords.called.should.equal(false) - }) - }) - describe('without a language', function () { beforeEach(function (done) { this.SpellingAPIManager.runRequest( @@ -158,131 +120,5 @@ describe('SpellingAPIManager', function () { .should.equal(true) }) }) - - describe('with words from the whitelist', function () { - beforeEach(function (done) { - this.whitelistWord = this.SpellingAPIManager.whitelist[0] - this.words = ['One', 'Two', this.whitelistWord] - this.SpellingAPIManager.runRequest( - this.token, - { words: this.words }, - (error, result) => { - if (error) return done(error) - this.result = result - done() - } - ) - }) - - it('should ignore the white-listed word', function () { - expect(this.result.misspellings.length).to.equal( - this.misspellings.length - 1 - ) - }) - }) - }) - - describe('learnWord', function () { - describe('without a token', function () { - beforeEach(function (done) { - this.SpellingAPIManager.learnWord(null, { word: 'banana' }, error => { - this.error = error - done() - }) - }) - - it('should return an error', function () { - expect(this.error).to.exist - expect(this.error).to.be.instanceof(Error) - expect(this.error.message).to.equal('no token provided') - }) - }) - - describe('without a word', function () { - beforeEach(function (done) { - this.SpellingAPIManager.learnWord(this.token, {}, error => { - this.error = error - done() - }) - }) - - it('should return an error', function () { - expect(this.error).to.exist - expect(this.error).to.be.instanceof(Error) - expect(this.error.message).to.equal('malformed JSON') - }) - }) - - describe('with a word and a token', function () { - beforeEach(function (done) { - this.word = 'banana' - this.SpellingAPIManager.learnWord( - this.token, - { word: this.word }, - error => { - this.error = error - done() - } - ) - }) - - it('should call LearnedWordsManager.learnWord', function () { - this.LearnedWordsManager.learnWord - .calledWith(this.token, this.word) - .should.equal(true) - }) - }) - }) - - describe('unlearnWord', function () { - describe('without a token', function () { - beforeEach(function (done) { - this.SpellingAPIManager.unlearnWord(null, { word: 'banana' }, error => { - this.error = error - done() - }) - }) - - it('should return an error', function () { - expect(this.error).to.exist - expect(this.error).to.be.instanceof(Error) - expect(this.error.message).to.equal('no token provided') - }) - }) - - describe('without a word', function () { - beforeEach(function (done) { - this.SpellingAPIManager.unlearnWord(this.token, {}, error => { - this.error = error - done() - }) - }) - - it('should return an error', function () { - expect(this.error).to.exist - expect(this.error).to.be.instanceof(Error) - expect(this.error.message).to.equal('malformed JSON') - }) - }) - - describe('with a word and a token', function () { - beforeEach(function (done) { - this.word = 'banana' - this.SpellingAPIManager.unlearnWord( - this.token, - { word: this.word }, - error => { - this.error = error - done() - } - ) - }) - - it('should call LearnedWordsManager.unlearnWord', function () { - this.LearnedWordsManager.unlearnWord - .calledWith(this.token, this.word) - .should.equal(true) - }) - }) }) }) diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index bc54d699b4..de7ba65ac2 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -696,7 +696,7 @@ const ProjectController = { if (!userId) { return cb(null, []) } - SpellingHandler.getUserDictionaryWithRetries(userId, cb) + SpellingHandler.getUserDictionary(userId, cb) }, subscription(cb) { if (userId == null) { diff --git a/services/spelling/app/js/LearnedWordsManager.js b/services/web/app/src/Features/Spelling/LearnedWordsManager.js similarity index 67% rename from services/spelling/app/js/LearnedWordsManager.js rename to services/web/app/src/Features/Spelling/LearnedWordsManager.js index 172a098e80..e8ea2f7d52 100644 --- a/services/spelling/app/js/LearnedWordsManager.js +++ b/services/web/app/src/Features/Spelling/LearnedWordsManager.js @@ -1,5 +1,4 @@ -const { db } = require('./mongodb') -const mongoCache = require('./MongoCache') +const { db } = require('../../infrastructure/mongodb') const logger = require('@overleaf/logger') const metrics = require('@overleaf/metrics') const { promisify } = require('util') @@ -7,10 +6,6 @@ const OError = require('@overleaf/o-error') const LearnedWordsManager = { learnWord(userToken, word, callback) { - if (callback == null) { - callback = () => {} - } - mongoCache.del(userToken) return db.spellingPreferences.updateOne( { token: userToken, @@ -26,10 +21,6 @@ const LearnedWordsManager = { }, unlearnWord(userToken, word, callback) { - if (callback == null) { - callback = () => {} - } - mongoCache.del(userToken) return db.spellingPreferences.updateOne( { token: userToken, @@ -42,26 +33,6 @@ const LearnedWordsManager = { }, getLearnedWords(userToken, callback) { - if (callback == null) { - callback = () => {} - } - const mongoCachedWords = mongoCache.get(userToken) - if (mongoCachedWords != null) { - metrics.inc('mongoCache', 0.1, { status: 'hit' }) - return callback(null, mongoCachedWords) - } - - metrics.inc('mongoCache', 0.1, { status: 'miss' }) - logger.info({ userToken }, 'mongoCache miss') - - LearnedWordsManager.getLearnedWordsNoCache(userToken, (err, words) => { - if (err) return callback(err) - mongoCache.set(userToken, words) - callback(null, words) - }) - }, - - getLearnedWordsNoCache(userToken, callback) { db.spellingPreferences.findOne( { token: userToken }, function (error, preferences) { @@ -82,9 +53,6 @@ const LearnedWordsManager = { }, deleteUsersLearnedWords(userToken, callback) { - if (callback == null) { - callback = () => {} - } db.spellingPreferences.deleteOne({ token: userToken }, callback) }, } diff --git a/services/web/app/src/Features/Spelling/SpellingController.js b/services/web/app/src/Features/Spelling/SpellingController.js index 0363770ff0..3cf3f2972d 100644 --- a/services/web/app/src/Features/Spelling/SpellingController.js +++ b/services/web/app/src/Features/Spelling/SpellingController.js @@ -2,6 +2,7 @@ const request = require('request') const Settings = require('@overleaf/settings') const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') +const LearnedWordsManager = require('./LearnedWordsManager') const TEN_SECONDS = 1000 * 10 @@ -9,6 +10,15 @@ const languageCodeIsSupported = code => Settings.languages.some(lang => lang.code === code) module.exports = { + learn(req, res, next) { + const { word } = req.body + const userId = SessionManager.getLoggedInUserId(req.session) + LearnedWordsManager.learnWord(userId, word, err => { + if (err) return next(err) + res.sendStatus(204) + }) + }, + proxyRequestToSpellingApi(req, res) { const { language } = req.body diff --git a/services/web/app/src/Features/Spelling/SpellingHandler.js b/services/web/app/src/Features/Spelling/SpellingHandler.js index 061a3e443a..9e3aa012a2 100644 --- a/services/web/app/src/Features/Spelling/SpellingHandler.js +++ b/services/web/app/src/Features/Spelling/SpellingHandler.js @@ -1,117 +1,26 @@ -const request = require('request') -const requestRetry = require('requestretry') -const Settings = require('@overleaf/settings') const OError = require('@overleaf/o-error') const Metrics = require('@overleaf/metrics') - -const TIMEOUT = 10 * 1000 +const LearnedWordsManager = require('./LearnedWordsManager') module.exports = { - getUserDictionaryWithRetries(userId, callback) { - const timer = new Metrics.Timer('spelling_get_dict') - const options = { - url: `${Settings.apis.spelling.url}/user/${userId}`, - timeout: 3 * 1000, - json: true, - retryDelay: 1, - maxAttempts: 3, - } - requestRetry(options, (error, response, body) => { - if (error) { - return callback( - OError.tag(error, 'error getting user dictionary', { error, userId }) - ) - } - - if (response.statusCode !== 200) { - return callback( - new OError( - 'Non-success code from spelling API when getting user dictionary', - { userId, statusCode: response.statusCode } - ) - ) - } - - timer.done() - callback(null, body) - }) - }, - getUserDictionary(userId, callback) { - const url = `${Settings.apis.spelling.url}/user/${userId}` - request.get({ url: url, timeout: TIMEOUT }, (error, response) => { + const timer = new Metrics.Timer('spelling_get_dict') + LearnedWordsManager.getLearnedWords(userId, (error, words) => { if (error) { return callback( OError.tag(error, 'error getting user dictionary', { error, userId }) ) } - - if (response.statusCode < 200 || response.statusCode >= 300) { - return callback( - new OError( - 'Non-success code from spelling API when getting user dictionary', - { userId, statusCode: response.statusCode } - ) - ) - } - - callback(null, JSON.parse(response.body)) + timer.done() + callback(null, words) }) }, deleteWordFromUserDictionary(userId, word, callback) { - const url = `${Settings.apis.spelling.url}/user/${userId}/unlearn` - request.post( - { - url: url, - json: { - word, - }, - timeout: TIMEOUT, - }, - (error, response) => { - if (error) { - return callback( - OError.tag(error, 'error deleting word from user dictionary', { - userId, - word, - }) - ) - } - - if (response.statusCode < 200 || response.statusCode >= 300) { - return callback( - new OError( - 'Non-success code from spelling API when removing word from user dictionary', - { userId, word, statusCode: response.statusCode } - ) - ) - } - - callback() - } - ) + LearnedWordsManager.unlearnWord(userId, word, callback) }, deleteUserDictionary(userId, callback) { - const url = `${Settings.apis.spelling.url}/user/${userId}` - request.delete({ url: url, timeout: TIMEOUT }, (error, response) => { - if (error) { - return callback( - OError.tag(error, 'error deleting user dictionary', { userId }) - ) - } - - if (response.statusCode < 200 || response.statusCode >= 300) { - return callback( - new OError( - 'Non-success code from spelling API when removing user dictionary', - { userId, statusCode: response.statusCode } - ) - ) - } - - callback() - }) + LearnedWordsManager.deleteUsersLearnedWords(userId, callback) }, } diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index d2ff8ce8f4..bc9a125486 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -847,8 +847,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) webRouter.post( '/spelling/learn', + validate({ + body: Joi.object({ + word: Joi.string().required(), + }), + }), AuthenticationController.requireLogin(), - SpellingController.proxyRequestToSpellingApi + SpellingController.learn ) webRouter.get( diff --git a/services/web/test/acceptance/src/LearnTest.js b/services/web/test/acceptance/src/LearnTest.js new file mode 100644 index 0000000000..92a920b5af --- /dev/null +++ b/services/web/test/acceptance/src/LearnTest.js @@ -0,0 +1,52 @@ +const { expect } = require('chai') +const cheerio = require('cheerio') +const User = require('./helpers/User').promises + +describe('Spelling', function () { + let user, projectId + async function learnWord(word) { + const { response } = await user.doRequest('POST', { + url: '/spelling/learn', + json: { word }, + }) + return response + } + + async function getDict() { + const { body, response } = await user.doRequest( + 'GET', + `/project/${projectId}` + ) + expect(response.statusCode).to.equal(200) + const dom = cheerio.load(body) + const metaEl = dom('meta[name="ol-learnedWords"]')[0] + return JSON.parse(metaEl.attribs.content) + } + + describe('learning words', function () { + beforeEach(async function () { + user = new User() + await user.login() + projectId = await user.createProject('foo') + }) + + it('should return status 400 when posting an empty word', async function () { + const response = await learnWord('') + expect(response.statusCode).to.equal(400) + }) + + it('should return status 204 when posting a word successfully', async function () { + const response = await learnWord('abcd') + expect(response.statusCode).to.equal(204) + }) + + it('should not learn the same word twice', async function () { + await learnWord('foobar') + const learnResponse = await learnWord('foobar') + expect(learnResponse.statusCode).to.equal(204) + + const dict = await getDict() + expect(dict.length).to.equals(1) + }) + }) +}) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 4722ff5cfa..cacd39511c 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -194,7 +194,7 @@ describe('ProjectController', function () { }, '../Helpers/NewLogsUI': this.NewLogsUIHelper, '../Spelling/SpellingHandler': { - getUserDictionaryWithRetries: sinon.stub().yields(null, []), + getUserDictionary: sinon.stub().yields(null, []), }, }, }) diff --git a/services/spelling/test/unit/js/LearnedWordsManagerTests.js b/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js similarity index 68% rename from services/spelling/test/unit/js/LearnedWordsManagerTests.js rename to services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js index 1fb0b0d874..4ce814d36e 100644 --- a/services/spelling/test/unit/js/LearnedWordsManagerTests.js +++ b/services/web/test/unit/src/Spelling/LearnedWordsManagerTests.js @@ -1,9 +1,9 @@ const sinon = require('sinon') -const { assert, expect } = require('chai') +const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const modulePath = require('path').join( __dirname, - '../../../app/js/LearnedWordsManager' + '/../../../../app/src/Features/Spelling/LearnedWordsManager' ) describe('LearnedWordsManager', function () { @@ -15,15 +15,9 @@ describe('LearnedWordsManager', function () { updateOne: sinon.stub().yields(), }, } - this.cache = { - get: sinon.stub(), - set: sinon.stub(), - del: sinon.stub(), - } this.LearnedWordsManager = SandboxedModule.require(modulePath, { requires: { - './mongodb': { db: this.db }, - './MongoCache': this.cache, + '../../infrastructure/mongodb': { db: this.db }, '@overleaf/metrics': { timeAsyncMethod: sinon.stub(), inc: sinon.stub(), @@ -106,39 +100,6 @@ describe('LearnedWordsManager', function () { }) }) - describe('caching the result', function () { - it('should use the cache first if it is primed', function (done) { - this.wordList = ['apples', 'bananas', 'pears'] - this.cache.get.returns(this.wordList) - this.db.spellingPreferences.findOne = sinon.stub() - this.LearnedWordsManager.getLearnedWords(this.token, (err, spellings) => { - expect(err).not.to.exist - this.db.spellingPreferences.findOne.called.should.equal(false) - assert.deepEqual(this.wordList, spellings) - done() - }) - }) - - it('should set the cache after hitting the db', function (done) { - this.wordList = ['apples', 'bananas', 'pears'] - this.db.spellingPreferences.findOne = sinon - .stub() - .callsArgWith(1, null, { learnedWords: this.wordList }) - this.LearnedWordsManager.getLearnedWords(this.token, () => { - this.cache.set.calledWith(this.token, this.wordList).should.equal(true) - done() - }) - }) - - it('should break cache when update is called', function (done) { - this.word = 'instanton' - this.LearnedWordsManager.learnWord(this.token, this.word, () => { - this.cache.del.calledWith(this.token).should.equal(true) - done() - }) - }) - }) - describe('deleteUsersLearnedWords', function () { beforeEach(function () { this.db.spellingPreferences.deleteOne = sinon.stub().callsArgWith(1) diff --git a/services/web/test/unit/src/Spelling/SpellingControllerTests.js b/services/web/test/unit/src/Spelling/SpellingControllerTests.js index e58b065dd4..c4b72fff3f 100644 --- a/services/web/test/unit/src/Spelling/SpellingControllerTests.js +++ b/services/web/test/unit/src/Spelling/SpellingControllerTests.js @@ -27,6 +27,7 @@ describe('SpellingController', function () { } this.controller = SandboxedModule.require(modulePath, { requires: { + './LearnedWordsManager': {}, request: this.request, '@overleaf/settings': { languages: [ diff --git a/services/web/test/unit/src/Spelling/SpellingHandlerTests.js b/services/web/test/unit/src/Spelling/SpellingHandlerTests.js deleted file mode 100644 index eebf3cc175..0000000000 --- a/services/web/test/unit/src/Spelling/SpellingHandlerTests.js +++ /dev/null @@ -1,124 +0,0 @@ -const SandboxedModule = require('sandboxed-module') -const { expect } = require('chai') -const sinon = require('sinon') -const modulePath = require('path').join( - __dirname, - '../../../../app/src/Features/Spelling/SpellingHandler.js' -) - -const TIMEOUT = 1000 * 10 - -const SPELLING_HOST = 'http://spelling.service.test' -const SPELLING_URL = 'http://spelling.service.test' - -describe('SpellingHandler', function () { - let userId, word, dictionary, dictionaryString, request, SpellingHandler - - beforeEach(function () { - userId = 'wombat' - word = 'potato' - dictionary = ['wombaat', 'woombat'] - dictionaryString = JSON.stringify(dictionary) - request = { - get: sinon - .stub() - .yields(null, { statusCode: 200, body: dictionaryString }), - post: sinon.stub().yields(null, { statusCode: 204 }), - delete: sinon.stub().yields(null, { statusCode: 204 }), - } - - SpellingHandler = SandboxedModule.require(modulePath, { - requires: { - request: request, - '@overleaf/settings': { - apis: { spelling: { host: SPELLING_HOST, url: SPELLING_URL } }, - }, - }, - }) - }) - - describe('getUserDictionary', function () { - it('calls the spelling API', function (done) { - SpellingHandler.getUserDictionary(userId, () => { - expect(request.get).to.have.been.calledWith({ - url: 'http://spelling.service.test/user/wombat', - timeout: TIMEOUT, - }) - done() - }) - }) - - it('returns the dictionary', function (done) { - SpellingHandler.getUserDictionary(userId, (err, dictionary) => { - expect(err).not.to.exist - expect(dictionary).to.deep.equal(dictionary) - done() - }) - }) - - it('returns an error when the request fails', function (done) { - request.get = sinon.stub().yields(new Error('ugh')) - SpellingHandler.getUserDictionary(userId, err => { - expect(err).to.exist - done() - }) - }) - }) - - describe('deleteWordFromUserDictionary', function () { - it('calls the spelling API', function (done) { - SpellingHandler.deleteWordFromUserDictionary(userId, word, () => { - expect(request.post).to.have.been.calledWith({ - url: 'http://spelling.service.test/user/wombat/unlearn', - json: { - word: word, - }, - timeout: TIMEOUT, - }) - done() - }) - }) - - it('does not return an error', function (done) { - SpellingHandler.deleteWordFromUserDictionary(userId, word, err => { - expect(err).not.to.exist - done() - }) - }) - - it('returns an error when the request fails', function (done) { - request.post = sinon.stub().yields(new Error('ugh')) - SpellingHandler.deleteWordFromUserDictionary(userId, word, err => { - expect(err).to.exist - done() - }) - }) - }) - - describe('deleteUserDictionary', function () { - it('calls the spelling API', function (done) { - SpellingHandler.deleteUserDictionary(userId, () => { - expect(request.delete).to.have.been.calledWith({ - url: 'http://spelling.service.test/user/wombat', - timeout: TIMEOUT, - }) - done() - }) - }) - - it('does not return an error', function (done) { - SpellingHandler.deleteUserDictionary(userId, err => { - expect(err).not.to.exist - done() - }) - }) - - it('returns an error when the request fails', function (done) { - request.delete = sinon.stub().yields(new Error('ugh')) - SpellingHandler.deleteUserDictionary(userId, err => { - expect(err).to.exist - done() - }) - }) - }) -})