add the internal updateGitMetadata api by GnsP · Pull Request #15340 · cdapio/cdap

GnsP · 2023-09-29T04:26:42Z

This PR adds an internal api api/v3internal/namespaces/:namespace/apps/updateSourceControlMeta. This api takes a list of appIds (with versionIds) with gitFileHashes and updates their scm metadata in the database.

JIRA: CDAP-20856

albertshau

can you explain why we need this?

albertshau · 2023-10-12T20:05:41Z

  }

+  @Override
+  public void updateApplicationSourceControlMeta(List<UpdateSourceControlMetaRequest> updateRequests) throws IOException {


I don't understand what the intent of this method is. As implemented, it looks like it is trying to create a new application version with the current app spec but with a different file hash. But why would we want to do that? The file hash is tied to the application information, if it is changing the application should also be changing.

I have updated the implementation to update only the source_control_metadata column.

albertshau · 2023-10-17T17:38:51Z

+   *
+   * @throws IOException if scm meta update fails
+   */
+  void updateApplicationSourceControlMeta(List<UpdateSourceControlMetaRequest> updateRequests)


List -> Collection

any time a store method is added, it must be covered by new unit tests.

Made the change to take a collection. And added unit test for the method.

albertshau · 2023-10-17T17:39:24Z

                                                             ApplicationSpecification specification);

+  /**
+   * updates source control meta of an application


Updates source control metadata for one or more applications

Like for other batch APIs, this should clearly describe the expected behavior in relevant edge cases. For example, what is the expected behavior if one of the applications in the give collection does not exist.

Updated the javadoc and added exception cases.

albertshau · 2023-10-17T17:42:04Z

+   * @throws Exception if either namespace or any of the application doesn't exist
+   */
+  @POST
+  @Path("/apps/updateGitMetadata")


in ApplicationDetail, it is called 'sourceControlMeta'. Should update this to be consistent, updateSourceControlMeta

albertshau · 2023-10-17T17:47:45Z

+    List<AppUpdateGitMetaRequest> appsUpdateGitMetaRequest;
+
+    try {
+      appsUpdateGitMetaRequest = parseBody(request, listOfAppUpdateGitMetaRequestType);


In general, request bodies should be objects and not collections, even if it is just an object with a single collection field. This allows the API to be more extensible. For example, in the future we could add an options field or something like that.

(This is not always followed in some of the older APIs, but those are bad examples).

albertshau · 2023-10-17T17:50:00Z

+      throws IOException {
+    List<UpdateSourceControlMetaRequest> updateScmMetaRequests = new ArrayList<>();
+    for (AppUpdateGitMetaRequest updateRequest : updateRequests) {
+      ApplicationId appId = new ApplicationId(


It is possible for this to throw an IllegalArgumentException for invalid application ids. Need to catch it and throw BadRequestException so that it becomes a 400.

albertshau · 2023-10-17T17:51:32Z

+import io.cdap.cdap.proto.id.ApplicationId;
+import java.util.Objects;
+
+public class UpdateSourceControlMetaRequest {


in general, the classes in proto represent something in the REST APIs. This should be moved internally to app-fabric.

moved to app-fabric. Renamed class to ApplicationSourceControlMeta.

albertshau · 2023-10-17T17:52:34Z

+import java.util.Objects;
+
+public class AppUpdateGitMetaRequest {
+  private final String appId;


make this consistent with ApplicationDetail.

appId -> name, fileHash -> sourceControlMeta

done. Also renamed the class to AppUpdateSourceControlMetaRequest.

albertshau · 2023-10-17T17:54:54Z

+  public void updateAppScmMeta(ApplicationId appId, SourceControlMeta scmMeta) throws IOException {
+    ApplicationMeta existing = getApplication(appId);
+    if (existing == null) {
+      throw new IllegalArgumentException("Application " + appId + " does not exist");


this shouldn't be IllegalArgumentException, ApplicaitonNotFoundException would be more specific.

Though I don't think throwing an exception is actually the behavior we want anyway. For example, if somebody deletes an application while the bulk push is happening, we probably just want to skip it instead of failing.

Updated to throw ApplicationNotFoundException.

I think we should throw the exception here. If we do not want to fail the atomic operation, then we can simply ignore this exception and continue with the transaction.

albertshau · 2023-10-17T17:56:52Z

+  @Test
+  public void testAppMarkLatestRequestDeserialize() throws Exception {
+    String appUpdateGitMetadataRequest = "{\n"
+        + "  \"appId\": \"test-application-id\","


as mentioned in the other PR, this is not a useful test that we need to perform, as it is basically just testing that Gson does the right thing.

Instead, there should be unit tests covering the new logic that was added.

removed this test.

samdgupi · 2023-10-19T10:14:58Z

+/**
+ * Request class to update sourceControlMeta of an application.
+ */
+public class AppUpdateSourceControlMetaRequest {


Please rename to UpdateSourceControlMetaRequest

samdgupi · 2023-10-19T10:23:52Z

  }
+
+  /**
+   * Update scm metadata for the given apps.


Provide context where this will be used.

added javadoc comment providing context on intended usage.

samdgupi · 2023-10-19T10:30:47Z

+  public void updateSourceControlMeta(NamespaceId namespace, AppsUpdateSourceControlMetaRequest appsRequest)
+      throws IOException, BadRequestException, ApplicationNotFoundException {
+
+    List<ApplicationSourceControlMeta> updateScmMetaRequests = new ArrayList<>();


nit: ApplicationSourceControlMeta class is strictly not needed. We would never need to extend this so we will be able to use a Map<ApplicationId, SourceControlMeta>

samdgupi · 2023-10-19T10:34:58Z

+    for (AppUpdateSourceControlMetaRequest appRequest : appsRequest.getApps()) {
+      String appName = appRequest.getName();
+      boolean isAdded = seenApps.add(appName);
+      if (!isAdded) {


single line

!(seenApps.add(appRequest.getName()))

samdgupi · 2023-10-19T10:39:55Z

+
+      try {
+        ApplicationId appId = namespace.app(appName, appRequest.getAppVersion());
+        SourceControlMeta scmMeta = new SourceControlMeta(appRequest.getSourceControlMeta());


change SourceControlMeta in request to gitFileHash

samdgupi · 2023-10-19T10:45:47Z

+        );
+        updateScmMetaRequests.add(updateScmMetaRequest);
+      } catch (IllegalArgumentException | NullPointerException e) {
+        throw new BadRequestException("Invalid application name or version. " + e.getCause());


the message should include the name , version and pass e as second parameter to the constructor.

samdgupi · 2023-10-19T10:50:55Z

+      AppMetadataStore mds = getAppMetadataStore(context);
+      for (ApplicationSourceControlMeta updateRequest : updateRequests) {
+        ApplicationId appId = updateRequest.getAppId();
+        mds.updateAppScmMeta(appId, updateRequest.getScmMeta());


ignore ApplicationNotFoundException and continue. In case the app isn missing we would still want to update other app metadata

samdgupi · 2023-10-19T10:51:55Z

  }

+  @Test
+  public void testUpdateApplicationScmMeta()


add error tests

samdgupi · 2023-10-19T10:55:57Z

+   * @throws BadRequestException when multiple filehashes are provided for an application
+   * @throws ApplicationNotFoundException when any of the applications is not found
+   */
+  public void updateSourceControlMeta(NamespaceId namespace, AppsUpdateSourceControlMetaRequest appsRequest)


Please add unit tests

Added unit tests for success and failure cases.

samdgupi · 2023-10-19T11:00:04Z

+/**
+ * Request class to update source control meta of multiple applications.
+ */
+public class AppsUpdateSourceControlMetaRequest {


UpdateMultiSourceControlMetaReqeust

albertshau

looks mostly good, but the comment about update not working correctly is concerning. Can you describe how it is broken?

albertshau · 2023-10-19T16:35:41Z

+              appRequest.getName(), appRequest.getAppVersion()
+          ));
+        }
+      } catch (IllegalArgumentException | NullPointerException e) {


this try-catch should be only around the namespace.app() call. That way it is clear what sort of error this is happening.

albertshau · 2023-10-19T16:37:03Z

+        }
+      } catch (IllegalArgumentException | NullPointerException e) {
+        throw new BadRequestException(
+            String.format("Invalid application name (%s) or version (%s).",


use the exception's message new BadRequestException(e.getMessage(), e). The cause already has a more specific error message about what is wrong.

albertshau · 2023-10-19T16:38:03Z

+   */
+  public void updateAppScmMeta(ApplicationId appId, SourceControlMeta scmMeta)
+      throws IOException, ApplicationNotFoundException {
+    final StructuredTable appSpecTable = getApplicationSpecificationTable();


doesn't need to be declared final

albertshau · 2023-10-19T16:40:38Z

+    }
+
+    fields.add(Fields.stringField(StoreDefinition.AppMetadataStore.SOURCE_CONTROL_META, GSON.toJson(scmMeta)));
+    // upsert, because update does not seem to work correctly in the nosql implementation


what does this mean, what does not work correctly?

It's a big problem if the nosql implementation is broken, would mean anything else using it is broken. If it is indeed broken, please open a blocker jira and reference it in the TODO below

Added a blocker JIRA to track this. CDAP-20853

GnsP · 2023-10-19T17:40:16Z

looks mostly good, but the comment about update not working correctly is concerning. Can you describe how it is broken?

Created the following JIRA to track this issue. The issue description can be found there.

https://cdap.atlassian.net/browse/CDAP-20853

albertshau

minor comments, otherwise lgtm.

Though it looks like the build is failing, please be sure to address any problems there.

albertshau · 2023-10-19T22:43:31Z

+
+      if (null == appRequest.getGitFileHash()) {
+        throw new BadRequestException(String.format(
+            "Null gitFileHash for app (name = %s, version = %s).",


nit: try to avoid more technical language like 'null' in user facing error messages. gitFileHash missing for app ...

it's also fine to combine the null and empty check into one if block.

albertshau · 2023-10-19T22:45:03Z

+
+      SourceControlMeta scmMeta = new SourceControlMeta(appRequest.getGitFileHash());
+
+      if (null != updateScmMetaRequests.put(appId, scmMeta)) {


nit: put the null on the other side of the equality check ... != null

GnsP added the build Triggers github actions build label Oct 11, 2023

albertshau reviewed Oct 12, 2023

View reviewed changes

samdgupi mentioned this pull request Oct 16, 2023

Implement SCM Pull LRO #15364

Merged

albertshau reviewed Oct 17, 2023

View reviewed changes

GnsP force-pushed the scm-update-git-meta branch from 16a41e1 to 9f8bcd9 Compare October 19, 2023 10:10

samdgupi suggested changes Oct 19, 2023

View reviewed changes

GnsP force-pushed the scm-update-git-meta branch from 9f8bcd9 to ce67d58 Compare October 19, 2023 12:44

albertshau reviewed Oct 19, 2023

View reviewed changes

GnsP force-pushed the scm-update-git-meta branch 2 times, most recently from cc2e8e9 to 0ecb43e Compare October 19, 2023 18:32

albertshau approved these changes Oct 19, 2023

View reviewed changes

GnsP force-pushed the scm-update-git-meta branch 2 times, most recently from a7eb8f1 to dd9f1c1 Compare October 20, 2023 05:16

samdgupi approved these changes Oct 23, 2023

View reviewed changes

add the internal updateGitMetadata api

505a87f

GnsP force-pushed the scm-update-git-meta branch from dd9f1c1 to 505a87f Compare October 24, 2023 03:42

samdgupi added the 6.10 label Oct 24, 2023

GnsP merged commit 1e709f6 into cdapio:develop Oct 24, 2023


		SourceControlMeta scmMeta = new SourceControlMeta(appRequest.getGitFileHash());

		if (null != updateScmMetaRequests.put(appId, scmMeta)) {

Conversation

GnsP commented Sep 29, 2023 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertshau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertshau Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

GnsP commented Sep 29, 2023 •

edited by atlassian Bot

Loading

albertshau Oct 17, 2023 •

edited

Loading