feat: mergepatch to support partial update of nested objects (#1891)

<!--
Follow semantic-release guidelines for the PR title, which is used in
the changelog.

Title should follow the format `<type>(<scope>): <subject>`, where
- Type is one of:
build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test|BREAKING
CHANGE
- Scope (optional) describes the place of the change (eg a particular
milestone) and is usually omitted
- subject should be a non-capitalized one-line description in present
imperative tense and not ending with a period

See
https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#-git-commit-guidelines
for more details.
-->

## Description
As discussed in issue #954, this PR implements support for
`application/merge-patch+json` type of content on requests to update
endpoints. Support is added to the newer V4 endpoints for now.

## Motivation
easier updates, especially of scientific metadata

## Changes:
<!-- Please provide a list of the changes implemented by this PR -->

* `application/merge-patch+json` content-type support
* `application/json` content-type remains
* in the controller function body it's checked what content-type user
sends in the request, and if it's merge-patch+json, additional merge
with the old document is applied
* changes are applied only to the new V4 endpoints for now
* for datasets, specific to ScientificMetadata and its interception,
additional check is added that doesn't allow updating either value/unit
without providing the second, when it was originally present in the
dataset, to avoid user errors and keep functionality of the interceptor
* eslint applied to jobs test files

## Tests included

- [x] Included for each change/fix?
- [ ] Passing? <!-- Merge will not be approved unless tests pass -->

## Documentation
- [x] swagger documentation updated (required for API changes)
- [ ] official documentation updated

### official documentation info
<!-- If you have updated the official documentation, please provide PR #
and URL of the updated pages -->

Author: sofyalaski

package-lock.json

@@ -46,6 +46,7 @@
     "@nestjs/platform-express": "^11",
     "@nestjs/swagger": "^11.1.1",
     "@nestjs/terminus": "^11",
+    "@types/json-merge-patch": "^1.0.0",
     "@user-office-software/duo-logger": "^2.1.1",
     "@user-office-software/duo-message-broker": "^1.4.0",
     "ajv": "^8.12.0",
@@ -58,6 +59,7 @@
     "express-session": "^1.17.3",
     "handlebars": "^4.7.7",
     "js-yaml": "^4.1.0",
+    "json-merge-patch": "^1.0.2",
     "jsonpath-plus": "^10.1.0",
     "lodash": "^4.17.21",
     "luxon": "^3.2.1",

package.json

@@ -19,13 +19,15 @@ import {
 import {
   ApiBearerAuth,
   ApiBody,
+  ApiConsumes,
   ApiOperation,
   ApiParam,
   ApiQuery,
   ApiResponse,
   ApiTags,
 } from "@nestjs/swagger";
 import { Request } from "express";
+import * as jmp from "json-merge-patch";
 import { PoliciesGuard } from "src/casl/guards/policies.guard";
 import { CheckPolicies } from "src/casl/decorators/check-policies.decorator";
 import { AppAbility, CaslAbilityFactory } from "src/casl/casl-ability.factory";
@@ -325,13 +327,14 @@ export class AttachmentsV4Controller {
   @ApiOperation({
     summary: "It updates the attachment.",
     description:
-      "It updates the attachment specified through the id specified. it updates only the specified fields.",
+      "It updates the attachment specified through the id specified. It updates only the specified fields. Set `content-type` to `application/merge-patch+json` if you would like to update nested objects. Warning! `application/merge-patch+json` doesn’t support updating a specific item in an array — the result will always replace the entire target if it’s not an object.",
   })
   @ApiParam({
     name: "aid",
     description: "ID of the attachment to modify",
     type: String,
   })
+  @ApiConsumes("application/json", "application/merge-patch+json")
   @ApiBody({
     type: PartialUpdateAttachmentV4Dto,
   })
@@ -347,14 +350,18 @@ export class AttachmentsV4Controller {
     @Param("aid") aid: string,
     @Body() updateAttachmentDto: PartialUpdateAttachmentV4Dto,
   ): Promise<OutputAttachmentV4Dto | null> {
-    await this.checkPermissionsForAttachment(
+    const foundAattachment = await this.checkPermissionsForAttachment(
       request,
       aid,
       Action.AttachmentUpdateEndpoint,
     );
+    const updateAttachmentDtoForservice =
+      request.headers["content-type"] === "application/merge-patch+json"
+        ? jmp.apply(foundAattachment, updateAttachmentDto)
+        : updateAttachmentDto;
     return this.attachmentsService.findOneAndUpdate(
       { _id: aid },
-      updateAttachmentDto,
+      updateAttachmentDtoForservice,
     );
   }
 

src/attachments/attachments.v4.controller.ts

@@ -216,9 +216,7 @@ export const mapScientificQuery = (
 /**Check if input is object or a physical quantity */
 const isObject = (x: unknown) => {
   if (
-    x &&
-    typeof x === "object" &&
-    !Array.isArray(x) &&
+    IsRecord(x) &&
     !(x as Record<string, unknown>).unit &&
     (x as Record<string, unknown>).unit !== "" &&
     !(x as Record<string, unknown>).u &&
@@ -229,6 +227,18 @@ const isObject = (x: unknown) => {
   return false;
 };
 
+export const IsRecord = (x: unknown): x is Record<string, unknown> => {
+  // checks if value is (nested) object
+  return x !== null && typeof x === "object" && !Array.isArray(x);
+};
+
+export const IsValueUnitObject = (
+  x: unknown,
+): x is { value?: unknown; unit?: unknown } => {
+  // checks if the argument is object and contains either property 'value' or 'unit'
+  return IsRecord(x) && ("value" in x || "unit" in x);
+};
+
 export const extractMetadataKeys = <T>(
   instances: T[],
   prop: keyof T,

src/common/utils.ts

@@ -14,13 +14,15 @@ import {
   HttpStatus,
   NotFoundException,
   Req,
+  BadRequestException,
   ForbiddenException,
   InternalServerErrorException,
   ConflictException,
 } from "@nestjs/common";
 import {
   ApiBearerAuth,
   ApiBody,
+  ApiConsumes,
   ApiExtraModels,
   ApiOperation,
   ApiParam,
@@ -30,6 +32,8 @@ import {
 } from "@nestjs/swagger";
 import { Request } from "express";
 import { MongoError } from "mongodb";
+import * as jmp from "json-merge-patch";
+import { IsRecord, IsValueUnitObject } from "../common/utils";
 import { DatasetsService } from "./datasets.service";
 import { DatasetClass, DatasetDocument } from "./schemas/dataset.schema";
 import { PoliciesGuard } from "src/casl/guards/policies.guard";
@@ -236,6 +240,47 @@ export class DatasetsV4Controller {
     return filter;
   }
 
+  findInvalidValueUnitUpdates(
+    updateDto: Record<string, unknown>,
+    dataset: Record<string, unknown>,
+    path: string[] = [],
+  ): string[] {
+    // collect properties that have both 'value' and 'unit' in original dataset but one of either is missing in the updateDto body
+    const unmatched: string[] = [];
+
+    for (const key in updateDto) {
+      const value = updateDto[key];
+      const currentPath = [...path, key];
+      if (IsValueUnitObject(value)) {
+        const datasetAtKey = currentPath.reduce<unknown>(
+          (obj, k) => (IsRecord(obj) ? obj[k] : undefined),
+          dataset,
+        );
+        if (IsValueUnitObject(datasetAtKey)) {
+          // check if current object's 'value' or 'unit' are not undefined in original dataset and passed updateDto
+          const originalHasValue = datasetAtKey.value !== undefined;
+          const originalHasUnit = datasetAtKey.unit !== undefined;
+          const updateHasValue = value.value !== undefined;
+          const updateHasUnit = value.unit !== undefined;
+          if (
+            originalHasValue &&
+            originalHasUnit &&
+            !(updateHasValue && updateHasUnit)
+          ) {
+            unmatched.push(currentPath.join("."));
+          }
+        }
+      }
+      // recursively go through the (scientificMetadata) object
+      if (IsRecord(value)) {
+        unmatched.push(
+          ...this.findInvalidValueUnitUpdates(value, dataset, currentPath),
+        );
+      }
+    }
+    return unmatched;
+  }
+
   // POST /api/v4/datasets
   @UseGuards(PoliciesGuard)
   @CheckPolicies("datasets", (ability: AppAbility) =>
@@ -670,13 +715,14 @@ export class DatasetsV4Controller {
   @ApiOperation({
     summary: "It partially updates the dataset.",
     description:
-      "It updates the dataset through the pid specified. It updates only the specified fields.",
+      "It updates the dataset through the pid specified. It updates only the specified fields. Set `content-type` to `application/merge-patch+json` if you would like to update nested objects. Warning! `application/merge-patch+json` doesn’t support updating a specific item in an array — the result will always replace the entire target if it’s not an object.",
   })
   @ApiParam({
     name: "pid",
     description: "Id of the dataset to modify",
     type: String,
   })
+  @ApiConsumes("application/merge-patch+json", "application/json")
   @ApiBody({
     description:
       "Fields that needs to be updated in the dataset. Only the fields that needs to be updated have to be passed in.",
@@ -705,11 +751,30 @@ export class DatasetsV4Controller {
       Action.DatasetUpdate,
     );
 
+    if (foundDataset && IsRecord(updateDatasetDto) && IsRecord(foundDataset)) {
+      const mismatchedPaths = this.findInvalidValueUnitUpdates(
+        updateDatasetDto,
+        foundDataset,
+      );
+      if (mismatchedPaths.length > 0) {
+        throw new BadRequestException(
+          `Original dataset ${pid} contains both value and unit in ${mismatchedPaths.join(", ")}. Please provide both when updating.`,
+        );
+      }
+    } else {
+      throw new BadRequestException(
+        `Failed to compare scientific metadata to include both value and units`,
+      );
+    }
+
+    const updateDatasetDtoForService =
+      request.headers["content-type"] === "application/merge-patch+json"
+        ? jmp.apply(foundDataset, updateDatasetDto)
+        : updateDatasetDto;
     const updatedDataset = await this.datasetsService.findByIdAndUpdate(
       pid,
-      updateDatasetDto,
+      updateDatasetDtoForService,
     );
-
     return updatedDataset;
   }
 

src/datasets/datasets.v4.controller.ts

@@ -17,6 +17,7 @@ import {
 } from "@nestjs/common";
 import { Request } from "express";
 import { FilterQuery } from "mongoose";
+import * as jmp from "json-merge-patch";
 import { JobsService } from "./jobs.service";
 import { CreateJobDto } from "./dto/create-job.dto";
 import { UpdateJobDto } from "./dto/update-job.dto";
@@ -33,6 +34,7 @@ import { OutputJobV3Dto } from "./dto/output-job-v3.dto";
 import {
   ApiBearerAuth,
   ApiBody,
+  ApiConsumes,
   ApiOperation,
   ApiQuery,
   ApiResponse,
@@ -791,12 +793,21 @@ export class JobsController {
     if (!canUpdate) {
       throw new ForbiddenException("Unauthorized to update this job.");
     }
+
     // Allow actions to validate DTO
     const validateContext = { request: updateJobDto, env: process.env };
     await validateActions(jobConfig.update.actions, validateContext);
 
+    const updateJobDtoForService =
+      request.headers["content-type"] === "application/merge-patch+json"
+        ? jmp.apply(currentJob, updateJobDto)
+        : updateJobDto;
+
     // Update job in database
-    const updatedJob = await this.jobsService.update(id, updateJobDto);
+    const updatedJob = await this.jobsService.update(
+      id,
+      updateJobDtoForService,
+    );
     // Perform the action that is specified in the update portion of the job configuration
     if (updatedJob !== null) {
       await this.checkConfigVersion(jobConfig, updatedJob);
@@ -851,8 +862,10 @@ export class JobsController {
   @Version("4")
   @ApiOperation({
     summary: "It updates an existing job.",
-    description: "It updates an existing job.",
+    description:
+      "It updates an existing job. Set `content-type` to `application/merge-patch+json` if you would like to update nested objects. Warning! `application/merge-patch+json` doesn’t support updating a specific item in an array — the result will always replace the entire target if it’s not an object.",
   })
+  @ApiConsumes("application/json", "application/merge-patch+json")
   @ApiBody({
     description: "Fields for the job to be updated",
     required: true,

src/jobs/jobs.controller.ts

@@ -10,6 +10,7 @@ import { Logger, ValidationPipe, VersioningType } from "@nestjs/common";
 import { ConfigService } from "@nestjs/config";
 import { AllExceptionsFilter, ScicatLogger } from "./loggers/logger.service";
 import { NestExpressApplication } from "@nestjs/platform-express";
+import * as bodyParser from "body-parser";
 
 async function bootstrap() {
   const app = await NestFactory.create<NestExpressApplication>(AppModule, {
@@ -86,7 +87,12 @@ async function bootstrap() {
     "maxFileUploadSizeInMb",
   );
 
-  app.useBodyParser("json", { limit: fileUploadLimitInMb });
+  app.use(
+    bodyParser.json({
+      type: ["application/json", "application/merge-patch+json"],
+      limit: fileUploadLimitInMb,
+    }),
+  );
   app.useBodyParser("urlencoded", {
     limit: fileUploadLimitInMb,
     extended: true,

src/main.ts

@@ -172,6 +172,46 @@ describe("Attachments v4 tests", () => {
           );
         });
     });
+
+    it("0410: should update attachment partially with nested properties with PATCH endpoint", async () => {
+      const updatePayload = {
+        relationships: [
+          {
+            targetId: "testId1-modified-twice",
+            targetType: "sample",
+          },
+          {
+            targetId: "testId2-modified-twice",
+            targetType: "sample",
+          },
+        ],
+      };
+
+      return request(appUrl)
+        .patch(`/api/v4/attachments/${encodeURIComponent(createdAttachmentId)}`)
+        .set("Content-type", "application/merge-patch+json")
+        .send(updatePayload)
+        .auth(accessTokenAdminIngestor, { type: "bearer" })
+        .expect(TestData.SuccessfulPatchStatusCode)
+        .expect("Content-Type", /json/)
+        .then((res) => {
+          res.body.should.be.a("object");
+          res.body.caption.should.equal("Updated caption text");
+          res.body.thumbnail.should.equal("Updated thumbnail URL");
+          res.body.relationships.should.deep.equal([
+            {
+              targetId: "testId1-modified-twice",
+              targetType: "sample",
+              relationType: "is attached to",
+            },
+            {
+              targetId: "testId2-modified-twice",
+              targetType: "sample",
+              relationType: "is attached to",
+            },
+          ]);
+        });
+    });
   });
 
   describe("Delete tests", () => {