perf: avoid recompute to optimize size updates

[minottic]

Description

The size and number of files was computed in a way that required the full datablocks scan by datasetId. The new implementation increments the sizes by simply adding the new entries. This is much more efficient

Tests included

• Included for each change/fix?
• Passing?

Documentation

• swagger documentation updated (required for API changes)
• official documentation updated

official documentation info

Summary by Sourcery

Optimize dataset size and file count maintenance by incrementally applying changes from datablocks and origdatablocks instead of recomputing aggregates.

New Features:

• Support incremental dataset size and file count updates when creating, updating, or deleting datablocks and origdatablocks.
• Expose new service and controller methods that operate on filters and pass old/new datablock documents for size update calculations.

Bug Fixes:

• Prevent dataset size/file updates when the targeted datablock or origdatablock is missing or disappears during update, returning appropriate HTTP exceptions instead.
• Ensure origdatablock and datablock update endpoints respect optimistic concurrency and permission checks while avoiding unintended dataset mutations.

Enhancements:

• Refine DatasetsService to compute and apply size and file count deltas directly on the dataset model using $inc operations.
• Improve service APIs for datablocks and origdatablocks to more clearly express combined update-plus-dataset-size maintenance flows.
• Strengthen mocking in origdatablocks unit tests to better cover create, update, and delete behaviors tied to dataset size updates.

Tests:

• Add unit tests for DatasetsService.updateDatasetSizeAndFiles to validate delta calculations for create, update, and delete scenarios.
• Extend datablocks and origdatablocks service and controller tests to cover incremental size updates and error handling when documents are missing.
• Introduce an integration test verifying dataset packedSize and numberOfFilesArchived stay consistent when a datablock is modified.

[sourcery-ai]

Hey - I've found 3 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/datablocks/datablocks.service.ts" line_range="117" />
<code_context>
+  async updateDatablockById(
</code_context>
<issue_to_address>
**issue (bug_risk):** Updating a datablock now increments dataset size/files by the new values only, which can overcount on updates.

Previously, dataset size and file count were recomputed via aggregation over all datablocks, so updates implicitly replaced the old values. With the new `$inc` approach, `updateDatablockById` calls `updateDatasetSizeAndFiles(datablock)` with only the new datablock values, without subtracting the old ones, so each update double-counts size and files. This path should instead compute a delta (new − old) or fetch the previous datablock and adjust the totals accordingly.
</issue_to_address>

### Comment 2
<location path="src/origdatablocks/origdatablocks.service.ts" line_range="420" />
<code_context>
+  async updateOrigDatablock(
</code_context>
<issue_to_address>
**issue (bug_risk):** Origdatablock updates similarly risk double-counting dataset size and file totals.

Here we have the same issue as in `DatablocksService`: `updateOrigDatablock` calls `updateDatasetSizeAndFiles(origDatablock)` with the updated origdatablock, so `$inc` applies the full new `size` and `numberOfFiles` each time. This repeatedly adds the new totals without subtracting the previous contribution, causing the dataset aggregates to drift. As with datablocks, this should compute and apply a delta between the old and new origdatablock values instead of incrementing by the new values alone.
</issue_to_address>

### Comment 3
<location path="src/datasets/datasets.service.ts" line_range="738-744" />
<code_context>
-      "packedSize",
-      "numberOfFilesArchived",
-    );
+  async updateDatasetSizeAndFiles(
+    datablock: Datablock,
+    remove = false,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `$inc` API for dataset size updates relies on callers always passing deltas, but the type and naming suggest absolute values.

The union type for `sizes` (`Pick<DatasetDocument, "size" | "numberOfFiles">` | `Pick<DatasetDocument, "packedSize" | "numberOfFilesArchived">`) suggests callers should pass full values, but `$inc: sizes` actually expects deltas. This makes it easy to mistakenly pass absolute totals and corrupt counters. Please either introduce a dedicated delta type (e.g., `DatasetSizeDelta`) with clear semantics or rename the parameter (e.g., `sizeDelta`) to better signal that deltas are required.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In DatablocksService.updateByIdAndUpdateDatasetSizeAndFileCount, accessing datablock.dataFileList.length assumes the array is always defined; to avoid potential runtime errors on partial updates, consider using optional chaining and defaulting (e.g., datablock.dataFileList?.length ?? 0) as done in OrigDatablocksService.
• DatasetsService.updateDatasetSizeAndFiles now applies a $inc delta but its name and sizesDelta parameter still suggest a full recomputation; consider renaming the method or parameter (e.g., updateDatasetSizesDelta) to make the incremental semantics clear and prevent accidental passing of absolute values.
• The custom MockOrigDatablockModel in origdatablocks.service.spec.ts implements a minimal subset of Model behavior; as the service evolves, centralizing or extending this mock (similar to the datablocks model mock) could reduce brittleness and keep tests aligned with actual Mongoose usage.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In DatablocksService.updateByIdAndUpdateDatasetSizeAndFileCount, accessing datablock.dataFileList.length assumes the array is always defined; to avoid potential runtime errors on partial updates, consider using optional chaining and defaulting (e.g., datablock.dataFileList?.length ?? 0) as done in OrigDatablocksService.
- DatasetsService.updateDatasetSizeAndFiles now applies a $inc delta but its name and sizesDelta parameter still suggest a full recomputation; consider renaming the method or parameter (e.g., updateDatasetSizesDelta) to make the incremental semantics clear and prevent accidental passing of absolute values.
- The custom MockOrigDatablockModel in origdatablocks.service.spec.ts implements a minimal subset of Model behavior; as the service evolves, centralizing or extending this mock (similar to the datablocks model mock) could reduce brittleness and keep tests aligned with actual Mongoose usage.

## Individual Comments

### Comment 1
<location path="src/datablocks/datablocks.service.ts" line_range="132-133" />
<code_context>
-    await this.updateDatasetSizeAndFiles(datablock.datasetId);
+    const packedSizeDelta =
+      (datablock.packedSize ?? 0) - (oldDatablock.packedSize ?? 0);
+    const numberOfFilesArchivedDelta =
+      (datablock.dataFileList.length ?? 0) -
+      (oldDatablock.dataFileList.length ?? 0);
+    if (packedSizeDelta !== 0 || numberOfFilesArchivedDelta !== 0) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against undefined `dataFileList` before accessing `.length` when computing deltas.

In `updateByIdAndUpdateDatasetSizeAndFileCount`, `numberOfFilesArchivedDelta` uses `(datablock.dataFileList.length ?? 0)` and `(oldDatablock.dataFileList.length ?? 0)`. If `dataFileList` is `undefined` or `null`, accessing `.length` will throw before `?? 0` applies. Please use `datablock.dataFileList?.length ?? 0` and `oldDatablock.dataFileList?.length ?? 0`, or ensure `dataFileList` is normalized to an empty array earlier.
</issue_to_address>

### Comment 2
<location path="src/datasets/datasets.controller.ts" line_range="2551" />
<code_context>
     );
     if (!dataset) throw new NotFoundException(`dataset: ${pid} not found`);

-    return this.datablocksService.updateAndUpdateDatasetSizeAndFileCount(
-      { _id: did, datasetId: pid },
+    return this.datablocksService.updateByIdAndUpdateDatasetSizeAndFileCount(
+      did,
       updateDatablockDto,
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping the `datasetId` constraint in the datablock update filter may allow cross-dataset updates.

Previously, the filter `{ _id: did, datasetId: pid }` guaranteed that the updated datablock belonged to the specified dataset. The new service call builds a filter only on `{ _id }`, so a caller can pass a `did` from another dataset and still have it updated after the `pid` existence check. If you need to enforce that the datablock is part of the given dataset (and avoid data integrity/authorization issues), add `datasetId: pid` back into the service filter or reapply this constraint in the controller.
</issue_to_address>

### Comment 3
<location path="src/origdatablocks/origdatablocks.service.spec.ts" line_range="50" />
<code_context>
   updateDatasetSizeAndFiles = jest.fn().mockResolvedValue(undefined);
 }

+function MockOrigDatablockModel(
+  this: Record<string, unknown>,
+  data: Record<string, unknown>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider reducing the mocked model surface and introducing small helper functions for repeated mock setup to make these tests easier to read and maintain.

You can simplify the mocking without changing behavior by tightening `MockOrigDatablockModel` to only what the service actually uses and by extracting small helpers for the repeated `findOne` / `findOneAndUpdate` / `findOneAndDelete` setup.

### 1. Trim `MockOrigDatablockModel` to used surface

Right now the mock adds several unused members (`find`, `deleteMany`, `countDocuments`, `aggregate`, `schema.path`), which makes reasoning about tests harder. For these tests you only need:

- constructor with `save`
- `findOne`
- `findOneAndUpdate`
- `findOneAndDelete`

You can keep the constructor semantics but drop the unused static members:

```ts
function MockOrigDatablockModel(
  this: Record<string, unknown>,
  data: Record<string, unknown>,
) {
  Object.assign(this, data);
  this.save = jest.fn().mockResolvedValue({ ...mockOrigDatablock, ...data });
}

MockOrigDatablockModel.findOne = jest.fn();
MockOrigDatablockModel.findOneAndUpdate = jest.fn();
MockOrigDatablockModel.findOneAndDelete = jest.fn();
```

If at some point a new test needs another method (e.g. `aggregate`), you can add it back in a focused way where it’s actually used.

### 2. Extract helpers for repeated mock setup

The `updateByIdAndUpdateDatasetSizeAndFileCount` and `removeAndUpdateDatasetSizeAndFileCount` tests repeat quite a bit of inline `findOne` / `findOneAndUpdate` / `findOneAndDelete` setup. Small helpers make this less noisy and easier to maintain:

```ts
function mockFindOne(orig: OrigDatablock | null) {
  (MockOrigDatablockModel.findOne as jest.Mock).mockResolvedValue(orig);
}

function mockFindOneAndUpdate(result: OrigDatablock | null) {
  (MockOrigDatablockModel.findOneAndUpdate as jest.Mock).mockReturnValue({
    exec: jest.fn().mockResolvedValue(result),
  });
}

function mockFindOneAndDelete(result: OrigDatablock | null) {
  (MockOrigDatablockModel.findOneAndDelete as jest.Mock).mockReturnValue({
    exec: jest.fn().mockResolvedValue(result),
  });
}
```

Then the tests become more readable:

```ts
it("should update the origdatablock and then update the dataset size and file count by the delta", async () => {
  mockFindOne(oldOrigDatablock);
  mockFindOneAndUpdate(mockOrigDatablock);

  const result = await service.updateByIdAndUpdateDatasetSizeAndFileCount(
    "testId",
    { size: mockOrigDatablock.size },
  );

  // assertions...
});

it("should throw NotFoundException and not touch the dataset when the origdatablock disappears during update", async () => {
  mockFindOne(oldOrigDatablock);
  mockFindOneAndUpdate(null);

  await expect(
    service.updateByIdAndUpdateDatasetSizeAndFileCount("testId", { size: 2000 }),
  ).rejects.toThrow(NotFoundException);
  // assertions...
});
```

These changes keep all current behavior intact (constructor usage, `save`, and the tested static methods) while reducing the mocking complexity and repeated inline setup.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

Hey - I've found 4 issues, and left some high level feedback:

• The new _updateDatasetSizeAndFiles helper uses a Parameters<typeof ...>[1] cast with loosely-typed deltas; consider introducing a dedicated SizesDelta type to make the expected keys explicit and avoid type assertions.
• Switching from findByIdAndUpdate (with OCC logic) to a raw updateOne + $inc for dataset size updates bypasses withOCCFilter; if OCC semantics are important for derived fields, you may want to preserve or explicitly document the changed concurrency behaviour.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `_updateDatasetSizeAndFiles` helper uses a `Parameters<typeof ...>[1]` cast with loosely-typed deltas; consider introducing a dedicated `SizesDelta` type to make the expected keys explicit and avoid type assertions.
- Switching from `findByIdAndUpdate` (with OCC logic) to a raw `updateOne` + `$inc` for dataset size updates bypasses `withOCCFilter`; if OCC semantics are important for derived fields, you may want to preserve or explicitly document the changed concurrency behaviour.

## Individual Comments

### Comment 1
<location path="src/datasets/datasets.service.ts" line_range="738-747" />
<code_context>
+  private async _updateDatasetSizeAndFiles(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The typing around `_updateDatasetSizeAndFiles` and the public `updateDatasetSizeAndFiles` is somewhat loose and could allow mismatched size/file fields at compile time.

The current signature of `updateDatasetSizeAndFiles` allows invalid key combinations (e.g. `size` with `numberOfFilesArchived`, or `packedSize` with `numberOfFiles`), and the cast to `Parameters<typeof this._updateDatasetSizeAndFiles>[1]` prevents the type system from catching this. Please tighten the typing (e.g. with overloads or a discriminated union) so only valid key pairs are representable and the `as` cast can be removed.

Suggested implementation:

```typescript
@Injectable({ scope: Scope.REQUEST })
export class DatasetsService {
  private readonly osDefaultIndex: string;
    }
  }

  /**
   * Represents the allowed combinations of size/file deltas for a dataset.
   * Only (size + numberOfFiles) or (packedSize + numberOfFilesArchived) are valid pairs.
   */
  type DatasetSizeAndFilesDelta =
    | {
        size: DatasetDocument["size"];
        numberOfFiles: DatasetDocument["numberOfFiles"];
        packedSize?: never;
        numberOfFilesArchived?: never;
      }
    | {
        packedSize: DatasetDocument["packedSize"];
        numberOfFilesArchived: DatasetDocument["numberOfFilesArchived"];
        size?: never;
        numberOfFiles?: never;
      };

  private async _updateDatasetSizeAndFiles(
    pid: string,
    sizesDelta: DatasetSizeAndFilesDelta,
  ): Promise<void> {

```

1. Update the public `updateDatasetSizeAndFiles` method to use the `DatasetSizeAndFilesDelta` type for its `sizesDelta` parameter instead of a loose union or `Parameters<typeof this._updateDatasetSizeAndFiles>[1]`.
   - Example shape: `public async updateDatasetSizeAndFiles(pid: string, sizesDelta: DatasetSizeAndFilesDelta): Promise<void> { return this._updateDatasetSizeAndFiles(pid, sizesDelta); }`
2. Remove any `as Parameters<typeof this._updateDatasetSizeAndFiles>[1]` casts when calling `_updateDatasetSizeAndFiles`; they should be unnecessary once both methods share the `DatasetSizeAndFilesDelta` type.
3. If `DatasetSizeAndFilesDelta` needs to be reused outside the class (e.g. in other services or controllers), move the type definition outside the class body or export it from a dedicated types file and update imports accordingly.
</issue_to_address>

### Comment 2
<location path="test/Datablock.js" line_range="212-221" />
<code_context>
+  it("0067: should update the packedSize and dataFileList of the first datablock and remove the old contribution while adding the new one", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a similar end-to-end test for origdatablocks to validate incremental size updates there as well.

Because the same incremental size logic now applies to origdatablocks via `OrigDatablocksService.updateDatasetSizeAndFiles`, please add a parallel e2e test for origdatablocks. Create an origdatablock, patch its `size` and `dataFileList`, and assert that the dataset’s `packedSize`/`size` and file counters correctly remove the old contribution and apply the new one, so both paths are covered end-to-end.

Suggested implementation:

```javascript
  it("0067: should update the packedSize and dataFileList of the first datablock and remove the old contribution while adding the new one", async () => {
    const updatedPackedSize = TestData.DataBlockCorrect.packedSize + 999;
    const updatedDataFileList = [TestData.DataBlockCorrect.dataFileList[0]];

    await request(appUrl)
      .patch(`/api/v3/datablocks/${datablockId}`)
      .send({
        packedSize: updatedPackedSize,
        dataFileList: updatedDataFileList,
      })
      .set("Accept", "application/json");
  });

  it("0068: should update the size and dataFileList of the first origdatablock and remove the old contribution while adding the new one", async () => {
    // Create an origdatablock so we have an initial contribution to the dataset
    const createOrigDatablockResponse = await request(appUrl)
      .post("/api/v3/origdatablocks")
      .send(TestData.OrigDatablockCorrect)
      .set("Accept", "application/json")
      .expect(201);

    const origDatablockId = createOrigDatablockResponse.body._id;
    const datasetId =
      createOrigDatablockResponse.body.datasetId ||
      TestData.OrigDatablockCorrect.datasetId ||
      TestData.DatasetCorrect.pid;

    // Fetch the dataset before patching the origdatablock to capture the initial contribution
    const datasetBeforeResponse = await request(appUrl)
      .get(`/api/v3/datasets/${datasetId}`)
      .set("Accept", "application/json")
      .expect(200);

    const datasetBefore = datasetBeforeResponse.body;
    const initialSize = datasetBefore.size;
    const initialPackedSize = datasetBefore.packedSize;
    const initialNumberOfFiles = datasetBefore.numberOfFiles;
    const initialNumberOfFilesArchived = datasetBefore.numberOfFilesArchived;

    const updatedSize = TestData.OrigDatablockCorrect.size + 999;
    const updatedDataFileList = [TestData.OrigDatablockCorrect.dataFileList[0]];

    // Patch the origdatablock: change size and dataFileList to trigger incremental update
    const patchOrigDatablockResponse = await request(appUrl)
      .patch(`/api/v3/origdatablocks/${origDatablockId}`)
      .send({
        size: updatedSize,
        dataFileList: updatedDataFileList,
      })
      .set("Accept", "application/json")
      .expect(200);

    // Fetch the dataset again after the patch to verify incremental update
    const datasetAfterResponse = await request(appUrl)
      .get(`/api/v3/datasets/${datasetId}`)
      .set("Accept", "application/json")
      .expect(200);

    const datasetAfter = datasetAfterResponse.body;

    // Assert that the dataset packedSize/size and file counters reflect removal of the old
    // origdatablock contribution and application of the new one
    expect(datasetAfter.size).to.not.equal(initialSize);
    expect(datasetAfter.packedSize).to.not.equal(initialPackedSize);

    // Size should be adjusted by (new - old) origdatablock size contribution
    expect(datasetAfter.size).to.equal(
      initialSize - TestData.OrigDatablockCorrect.size + updatedSize
    );

    // Packed size should similarly reflect removal of the old contribution and addition of the new one
    if (typeof datasetBefore.packedSize === "number") {
      expect(datasetAfter.packedSize).to.equal(
        initialPackedSize -
          (TestData.OrigDatablockCorrect.packedSize || TestData.OrigDatablockCorrect.size) +
          (patchOrigDatablockResponse.body.packedSize || updatedSize)
      );
    }

    // File counters should be adjusted based on the change in dataFileList length
    const originalFileCount = TestData.OrigDatablockCorrect.dataFileList.length;
    const updatedFileCount = updatedDataFileList.length;
    const fileCountDelta = updatedFileCount - originalFileCount;

    if (typeof initialNumberOfFiles === "number") {
      expect(datasetAfter.numberOfFiles).to.equal(
        initialNumberOfFiles + fileCountDelta
      );
    }

    if (typeof initialNumberOfFilesArchived === "number") {
      // If origdatablocks contribute to archived file counters, assert similarly
      expect(datasetAfter.numberOfFilesArchived).to.equal(
        initialNumberOfFilesArchived + fileCountDelta
      );
    }
  });

```

To align this new origdatablock e2e test with the existing test suite, you will likely need to:

1. **Confirm endpoints and payloads**:
   - Verify that `POST /api/v3/origdatablocks` and `PATCH /api/v3/origdatablocks/:id` are the correct endpoints.
   - Ensure `TestData.OrigDatablockCorrect` exists and contains `datasetId`, `size`, `dataFileList`, and possibly `packedSize`. If your test data uses different names or helpers, adjust accordingly.

2. **Use your existing assertion style**:
   - The file may be using `chai.expect`, `assert`, or another assertion library. Make sure the `expect(...)` calls match the imports and style already used in `test/Datablock.js`.

3. **Match dataset field names**:
   - Confirm the dataset properties used for counters: if your dataset model uses different names (e.g. `size`, `packedSize`, `totalFiles`, `archivedFiles`, etc.), update the assertions to match.
   - If origdatablocks only affect `size` and not `packedSize` (or vice versa), narrow the assertions to the fields actually updated by `OrigDatablocksService.updateDatasetSizeAndFiles`.

4. **Align with existing incremental logic**:
   - If other tests in this file calculate the expected deltas for dataset size and file counters using shared helpers or specific formulas, reuse that pattern rather than the inline calculations shown here.
   - You may need to derive the original origdatablock contribution from the `createOrigDatablockResponse.body` rather than `TestData.OrigDatablockCorrect` if the service normalizes or overrides some fields.

5. **Placement within the describe block**:
   - Ensure the new `it("0068: ...")` block is placed within the same `describe` as the datablock tests (or in the appropriate describe for origdatablocks if one already exists), and that the surrounding braces/semicolons are correct after inserting this patch.
</issue_to_address>

### Comment 3
<location path="src/datasets/datasets.service.ts" line_range="749" />
<code_context>
-    return result
-      ? { numberOfFiles: result.numberOfFiles, size: result.size }
-      : { numberOfFiles: 0, size: 0 };
+  async updateDatasetSizeAndFiles<T extends Datablock | OrigDatablock>(
+    pid: string,
+    sizeKey: Extract<keyof T, "size" | "packedSize">,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `updateDatasetSizeAndFiles` by removing the generic type, helper method, and `Parameters<>` cast in favor of a straightforward delta type or two explicit, strongly-typed methods.

The added generics, helper, and `Parameters<>` cast do increase complexity without much benefit. You can keep the `$inc`-based behavior while simplifying the API and types.

You can drop the generic `T` and the private helper entirely and use a simple delta type with a direct `$inc`:

```ts
type SizeDelta = {
  size?: number;
  packedSize?: number;
  numberOfFiles?: number;
  numberOfFilesArchived?: number;
};

async updateDatasetSizeAndFiles(
  pid: string,
  sizeKey: "size" | "packedSize",
  numberOfFilesKey: "numberOfFiles" | "numberOfFilesArchived",
  newDocument?: Datablock | OrigDatablock,
  oldDocument?: Datablock | OrigDatablock,
): Promise<void> {
  const newSize = (newDocument?.[sizeKey] ?? 0) as number;
  const newFiles = newDocument?.dataFileList?.length ?? 0;
  const oldSize = (oldDocument?.[sizeKey] ?? 0) as number;
  const oldFiles = oldDocument?.dataFileList?.length ?? 0;

  const delta: SizeDelta = {
    [sizeKey]: newSize - oldSize,
    [numberOfFilesKey]: newFiles - oldFiles,
  };

  await this.datasetModel.updateOne({ _id: pid }, { $inc: delta }).exec();
}
```

Alternatively, if you prefer stronger typing per use case, you can provide two explicit methods instead of a generic one:

```ts
async updateDatasetOrigSizes(
  pid: string,
  newDocument?: OrigDatablock,
  oldDocument?: OrigDatablock,
): Promise<void> {
  const newSize = newDocument?.size ?? 0;
  const newFiles = newDocument?.dataFileList?.length ?? 0;
  const oldSize = oldDocument?.size ?? 0;
  const oldFiles = oldDocument?.dataFileList?.length ?? 0;

  await this.datasetModel.updateOne(
    { _id: pid },
    { $inc: { size: newSize - oldSize, numberOfFiles: newFiles - oldFiles } },
  ).exec();
}

async updateDatasetPackedSizes(
  pid: string,
  newDocument?: Datablock,
  oldDocument?: Datablock,
): Promise<void> {
  const newSize = newDocument?.packedSize ?? 0;
  const newFiles = newDocument?.dataFileList?.length ?? 0;
  const oldSize = oldDocument?.packedSize ?? 0;
  const oldFiles = oldDocument?.dataFileList?.length ?? 0;

  await this.datasetModel.updateOne(
    { _id: pid },
    {
      $inc: {
        packedSize: newSize - oldSize,
        numberOfFilesArchived: newFiles - oldFiles,
      },
    },
  ).exec();
}
```

Both options keep the new delta-based behavior but avoid the generic `T`, `Extract<>`, private helper, and `Parameters<typeof ...>[1]` cast, making the code easier to understand and maintain.
</issue_to_address>

### Comment 4
<location path="src/origdatablocks/origdatablocks.service.spec.ts" line_range="50" />
<code_context>
   updateDatasetSizeAndFiles = jest.fn().mockResolvedValue(undefined);
 }

+function MockOrigDatablockModel(
+  this: Record<string, unknown>,
+  data: Record<string, unknown>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the shared MockOrigDatablockModel into a per-test, minimal mock that only defines the methods actually exercised by the service to avoid unnecessary global state and surface area.

The shared, hand-rolled `MockOrigDatablockModel` function with many unused statics is adding avoidable complexity and global state. You can keep all current behavior while making the model mock:

- local to each test run (no global mutation),
- focused only on the methods actually used in the service,
- clearer about instance vs. static behavior.

Example refactor:

```ts
describe("OrigdatablocksService", () => {
  let service: OrigDatablocksService;
  let datasetsService: DatasetsServiceMock;
  let MockOrigDatablockModel: any;

  beforeEach(async () => {
    const mockConstructor = function (this: any, data: Record<string, unknown>) {
      Object.assign(this, { ...mockOrigDatablock, ...data });
      this.save = jest
        .fn()
        .mockResolvedValue({ ...mockOrigDatablock, ...data });
    };

    MockOrigDatablockModel = mockConstructor;
    MockOrigDatablockModel.findOne = jest.fn();
    MockOrigDatablockModel.findOneAndUpdate = jest.fn();
    MockOrigDatablockModel.findOneAndDelete = jest.fn();

    const module: TestingModule = await Test.createTestingModule({
      providers: [
        OrigDatablocksService,
        {
          provide: getModelToken("OrigDatablock"),
          useValue: MockOrigDatablockModel,
        },
        { provide: DatasetsService, useClass: DatasetsServiceMock },
        { provide: REQUEST, useValue: { user: { username: "testUser" } } },
      ],
    }).compile();

    service = module.get(OrigDatablocksService);
    datasetsService = module.get(DatasetsServiceMock);
  });
```

Then tests override only what they actually use, without unused static methods:

```ts
(MockOrigDatablockModel.findOne as jest.Mock).mockResolvedValue(oldOrigDatablock);
(MockOrigDatablockModel.findOneAndUpdate as jest.Mock).mockReturnValue({
  exec: jest.fn().mockResolvedValue(mockOrigDatablock),
});
```

If you don’t need `deleteMany`, `countDocuments`, `aggregate`, or `schema.path` in this spec, you can completely drop them to reduce noise:

```ts
// Remove these unless they are referenced in the service code exercised here:
// MockOrigDatablockModel.deleteMany = jest.fn();
// MockOrigDatablockModel.countDocuments = jest.fn();
// MockOrigDatablockModel.aggregate = jest.fn();
// MockOrigDatablockModel.schema = { path: jest.fn().mockReturnValue({ instance: "String" }) };
```

This keeps the constructor/save behavior intact, but makes the mock minimal and per-test, so you no longer rely on global mutation and extra unused Mongoose-like surface area.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[minottic]

this as well as the origs update is not transactional. This is solved by this PR #2864 and adding the
@Transactional decorator to the method then