Ensure Compliance with AIP Rules for Resource Creation and Update

[minwoox]

Motivation:
Identified violations of two AIP (API Improvement Proposals) rules:

• CREATE: If a user tries to create a resource with an ID that would result in a duplicate resource name, the service must return an ALREADY_EXISTS error.
• UPDATE: If the method call is on a resource that already exists and all fields match, the existing resource should be returned unchanged.

Modifications:

• Implemented the aforementioned AIP rules during resource creation and update.
• Added `ControlPlaneExceptionHandlerFunction``, which converts exceptions raised within Central Dogma to appropriate gRPC status codes.

Result:

• The service now correctly returns an ALREADY_EXISTS error for duplicate resource creation attempts.
• Resource updates that do not change any fields will now correctly return the existing resource without modification.

[jrhee17]

👍 👍

[jrhee17]

Question) I'm wondering if there could be a race condition of two create requests are called concurrently, then the result would be a merged resource instead of the resource from either request.

e.g.

request1 -> addOperation("field1")
request2 -> addOperation("field2")

-> the resulting stored value has both "field1" and "field2"

1. Is this scenario possible? 2) I'm curious why it is necessary to use ofJsonPatch

[minwoox]

Is this scenario possible?

It's not possible.

I'm curious why it is necessary to use ofJsonPatch

Change.ofJsonPatch(fileName, null, jsonText); means that the commit must be made only when the fileName doesn't exist. You can check it here how it works:

• https://github.com/line/centraldogma/blob/main/common/src/main/java/com/linecorp/centraldogma/common/Change.java#L223
• https://github.com/line/centraldogma/blob/main/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java#L1107-L1116

Without ofJsonPatch, when a user creates multiple resources with the same ID, the last commit will override the previous one even though it should fail.

[jrhee17]

I see, I missed the following point:

centraldogma/common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/SafeReplaceOperation.java

Lines 55 to 56 in 7add9c2

	throw new JsonPatchException("mismatching value at '" + path + "': " +
	actual + " (expected: " + oldValue + ')');

Sounds good to me 👍

[codecov]

Codecov Report

Attention: Patch coverage is 56.52174% with 20 lines in your changes missing coverage. Please review.

Project coverage is 71.63%. Comparing base (f810f16) to head (2c1f803).
Report is 8 commits behind head on main.

Files	Patch %	Lines
...internal/ControlPlaneExceptionHandlerFunction.java	16.66%	9 Missing and 1 partial ⚠️
.../centraldogma/xds/internal/XdsResourceManager.java	77.77%	2 Missing and 2 partials ⚠️
...s/k8s/v1/XdsKubernetesEndpointFetchingService.java	0.00%	4 Missing ⚠️
...orp/centraldogma/xds/group/v1/XdsGroupService.java	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1025      +/-   ##
============================================
+ Coverage     71.60%   71.63%   +0.02%     
- Complexity     4121     4136      +15     
============================================
  Files           402      403       +1     
  Lines         16452    16505      +53     
  Branches       1762     1776      +14     
============================================
+ Hits          11781    11823      +42     
- Misses         3640     3649       +9     
- Partials       1031     1033       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ikhoon]

👍👍