mongoose pre hook 에러 추적기 (2)

1. 들어가며

이전 글에서 에러 추적을 통해 원인과 해야할 작업을 파악했으니, 이제 issue와 PR을 올려보려고 합니다.
이전 글에서 분석했을 때는 모든 mongoose middleware에 대해서 대응하려고 했지만, 전략적으로 기여하려고 합니다.

2. 전략

기여 전략은 아래와 같이 설정 하였습니다.
- 투스텝으로 진행
  - 첫번째, aggregate middleware만 대응
  - 두번째, 나머지 middleware도 대응
- aggregate에 대한 issue 생성
  - minimum reproduction 레포를 작성해서 Steps to Reproduce 단계를 명확하게 표현합니다.
  - 기존에 있던 이슈+PR 명시해서 메인테이너가 히스토리를 쉽게 파악하도록 합니다.
- aggreate에 대한 PR 올리기
  - Description에 추가적으로 내 생각에는 다른 것도 지원해야할 것 같다. 맞다면 PR 이어서 올리겠다 도 명시합니다.

3. issue 생성하기

hook is not of type Aggregate · Issue #14903 · Automattic/mongoose

Prerequisites I have written a descriptive issue title I have searched existing issues to ensure the bug has not already been reported Mongoose version 8.6.1 Node.js version 20.10.0 MongoDB server ...

아무래도 올라와 있는 issue가 아니기 때문에 직접 issue를 올렸습니다.
컨텍스트에 대해서 몇일간 작업한 저는 잘 알지만, 처음 해당 이슈를 접하는 사람들은 이슈에 대해서 잘 파악하기 어려울 것입니다.
- 따라서 쉽게 파악할 수 있도록 유사한 히스토리에 대해서 명시하고, reproduction repository를 생성해서 재현 코드를 작성하였습니다.

4. 해결하기

'use strict';

const middlewareFunctions = require('../../constants').queryMiddlewareFunctions;
const promiseOrCallback = require('../promiseOrCallback');

module.exports = function applyStaticHooks(model, hooks, statics) {
  const kareemOptions = {
    useErrorHandlers: true,
    numCallbackParams: 1
  };

  hooks = hooks.filter(hook => {
    // If the custom static overwrites an existing query middleware, don't apply
    // middleware to it by default. This avoids a potential backwards breaking
    // change with plugins like `mongoose-delete` that use statics to overwrite
    // built-in Mongoose functions.
    if (middlewareFunctions.indexOf(hook.name) !== -1) {
      return !!hook.model;
    }
    return hook.model !== false;
  });

  model.$__insertMany = hooks.createWrapper('insertMany',
    model.$__insertMany, model, kareemOptions);

  for (const key of Object.keys(statics)) {
    ...
  }
};

문제는 hooks.filter하는 부분에 있어서 query middleware와 동일한 이름을 가진 static method들만 필터하기 때문에 발생합니다.
query middleware과 동일한 이름을 가진 static method (e.g find(), findOne())의 경우는 pre/post 훅이 적용되지 않습니다.
- pre/post 훅이 호출되지 않으니 pre/post의 this context가 의도와 다르게 동작하는 경우는 없습니다.
하지만 query middleware을 제외한 다른 middleware와 동일한 이름을 가진 static method의 경우는 필터링되지 않아서 pre/post 훅이 적용됩니다.
- 이렇게 되면 pre/post 훅이 적용되는데, 이러한 pre/post 훅의 this context는 static method이기 때문에 의도한 context(e.g Aggregate)가 아니게 됩니다.

해결 방법

lib/constants.js

'use strict';
/*!
 * ignore
 */
const queryOperations = Object.freeze([
  // Read
  'countDocuments',
  'distinct',
  'estimatedDocumentCount',
  'find',
  'findOne',
  // Update
  'findOneAndReplace',
  'findOneAndUpdate',
  'replaceOne',
  'updateMany',
  'updateOne',
  // Delete
  'deleteMany',
  'deleteOne',
  'findOneAndDelete'
]);
exports.queryOperations = queryOperations;

/*!
 * ignore
 */
const queryMiddlewareFunctions = queryOperations.concat([
  'validate'
]);

exports.queryMiddlewareFunctions = queryMiddlewareFunctions;

/*!
 * ignore
 */

// 추가된 코드
const aggregateMiddlewareFunctions = [
  'aggregate'
];

exports.aggregateMiddlewareFunctions = aggregateMiddlewareFunctions;

다음과 같이 aggregateMiddlewareFunctions 배열을 추가합니다.

lib/helpers/model/applyStaticHooks.js

'use strict';

const promiseOrCallback = require('../promiseOrCallback');
const { queryMiddlewareFunctions, aggregateMiddlewareFunctions } = require('../../constants');

// 추가된 코드
const middlewareFunctions = [
  ...queryMiddlewareFunctions,
  ...aggregateMiddlewareFunctions
];

module.exports = function applyStaticHooks(model, hooks, statics) {
  const kareemOptions = {
    useErrorHandlers: true,
    numCallbackParams: 1
  };

  hooks = hooks.filter(hook => {
    // If the custom static overwrites an existing query/aggregate middleware, don't apply
    // middleware to it by default. This avoids a potential backwards breaking
    // change with plugins like `mongoose-delete` that use statics to overwrite
    // built-in Mongoose functions.
    if (middlewareFunctions.indexOf(hook.name) !== -1) {
      return !!hook.model;
    }
    return hook.model !== false;
  });

  model.$__insertMany = hooks.createWrapper('insertMany',
    model.$__insertMany, model, kareemOptions);

  for (const key of Object.keys(statics)) {
    ...
  }
};

queryMiddlewareFunctions와 aggregateMiddlewareFunctions를 합친 middlewareFunctions 배열을 만듭니다.
hooks.filter 로직에서 필터링되어 aggregate middleware과 동일한 이름을 가진 static method 또한 pre/post 훅이 적용되지 않습니다.

5. 해결한 부분을 테스트하기

이제 수정한 코드에 대해서 테스트 코드를 작성하고, 테스트 코드를 통과하는지 확인해야합니다.

테스트

우선 기존 테스트를 돌리고, 모두 통과하는지 확인합니다.

$ npm run test

모두 통과하는 것을 확인할 수 있습니다.

테스트 추가

it('custom statics that overwrite aggregate functions dont get hooks by default (gh-14903)', async function() {

    const schema = new Schema({ name: String });

    schema.statics.aggregate = function(pipeline) {
      return model.aggregate.apply(this, [pipeline]);
    };

    let called = 0;
    schema.pre('aggregate', function(next) {
      ++called;
      next();
    });
    const Model = db.model('Test', schema);

    await Model.create({ name: 'foo' });

    const res = await Model.aggregate([
      {
        $match: {
          name: 'foo'
        }
      }
    ]);

    assert.ok(res[0].name);
    assert.equal(called, 1);
  });

내가 수정한 코드가 잘 작동하는지 확인하기 위해 테스트에 위와 같은 코드를 추가하였습니다.
aggregate라는 이름을 가진 static method를 만들고, 해당 메서드에 pre 훅을 걸어두었습니다.
이때 called는 한번만 호출되어야 합니다.
- 한번 호출되는 것은 aggregate라는 static method에서 호출되는 것이 아니라, 해당 static method에서 호출하는 mongoose 메서드인 aggregate 메서드로 인해 호출됩니다.

테스트 확인

똑같이 테스트를 돌리고, 모두 통과하는지 확인합니다.

추가한 테스트도 모두 통과하는 것을 확인하였습니다. 이제 이제 PR을 올리겠습니다!

6. PR을 작성하기

fix(model): skip applying static hooks by default if static name conflicts with aggregate middleware by dragontaek-lee · Pull Request #14904 · Automattic/mongoose

Fix #14903 Summary Make it so that if the custom static overwrites an existing aggregate middleware, don't apply middleware to it by default. This should be achieved by filtering custom static-...

작성한 코드에 대해 PR을 작성하였습니다.
상황을 최대한 간결하게 명확하게 설명하고, 이해를 돕기 위해 issue에 적었던 상세 예시들을 다시 한번 적어두었습니다.

8. 기다리기

메인테이너분들의 반응을 기다립니다!
시차도, 언어도 다르기 때문에 명확한 커뮤니케이션이 참 중요하다는 것을 다시 한번 배우게 되었습니다.

9. 기여 완료!

PR이 머지되고 aggregate 뿐만 아니라 다른 middleware에 대한 작업 제안도 받아들여졌습니다.
컨트리뷰터! 👍

10. 후원하기

돈받지 않고 오픈소스를 운영하고 생태계를 발전시키는 분들에게 감사한 마음을 전달드렸습니다.

프로필에 뱃지도 생긴다는 점도 좋았습니다.

11. 마치며

제가 오픈소스에 관해 직접 이슈를 제기하고 PR을 올려서 작업한 것은 처음인데요, 여러모로 의미가 있었던 작업이였던 것 같습니다.
회사에서 운영하는 프로덕트에서 발생한 문제에 대해서 수동적으로 대응하지 않고 능동적으로 문제 해결을 했다는 점에서 의미가 큰 것 같습니다.
멘토링 때 오픈소스를 사용함에 있어서 공식 문서만 읽는 사람과 코드를 직접 보고 기여하는 사람의 차이는 현재는 잘 느껴지지 않을지 모르지만, 시간이 흐를수록 그 가치는 커진다고 하셨습니다.
- 해당 말이 크게 와닿았고 공감을 많이 하며 더 노력해야겠다는 생각을 했습니다.
- 많은 사람들이 모여서 작업하는 오픈소스 특성상 문서와 코드가 잘못되어있을 확률도 있어서, 작성된 코드를 보고 이해하는 능력은 중요한 것 같습니다.
- 또한 직접 코드를 학습하면서 오픈소스를 깊이 이해하고, 어떻게 더 효율적으로 활용할 수 있을지 고민하고 실행하는 것이 개발자로서 가장 큰 가치를 제공하는 것 같습니다.

← 이전글

mongoose pre hook 에러 추적기 (1)

mongoose pre hook 에러 추적기 (3)