A comprehensive error handling strategy of pulling entities -- part II #528

YannanGao-gs · 2022-10-04T20:45:22Z

Add new endpoints for pulling invalid entities.
If there is no invalid entity, the return value will be [].

Screen.Recording.2022-10-05.at.11.31.03.AM.mov

github-actions · 2022-10-04T20:53:57Z

Test Results

  98 files -   9   98 suites - 9 4m 2s ⏱️ - 1m 30s
498 tests - 50 497 ✔️ - 50 1 💤 ±0 0 ❌ ±0

Results for commit 184c8b1. ± Comparison against base commit 07e86c0.

This pull request removes 50 tests.

org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testEmptyEntitiesDirectory
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testFullModel
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testFullModelWithExcludedEntities
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testFullModelWithExcludedEntitiesAndPackages
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testFullModelWithExcludedPackages
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerationMojo ‑ testNoTestables
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerator ‑ testInvalidRootPackage
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerator ‑ testNonTestable
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerator ‑ testRelationalMapping
org.finos.legend.sdlc.test.junit.TestJUnitTestGenerator ‑ testServiceStoreMapping
…

♻️ This comment has been updated with latest results.

kevin-m-knight-gs

There are three big things here. First, these methods that return invalid entities cannot return type Entity. We will need a new type for this. Second, we also need APIs to get individual invalid entities by path. Third, the parameters used to filter on anything other than the entity path don't make sense when looking for invalid entities, since we may not be able to get any further information.

kevin-m-knight-gs · 2022-10-05T18:15:18Z

...server/src/main/java/org/finos/legend/sdlc/server/domain/api/entity/EntityAccessContext.java

@@ -26,5 +26,7 @@

    List<Entity> getEntities(Predicate<String> entityPathPredicate, Predicate<String> classifierPathPredicate, Predicate<? super Map<String, ?>> entityContentPredicate);

+    List<Entity> getInvalidEntities(Predicate<String> entityPathPredicate, Predicate<String> classifierPathPredicate, Predicate<? super Map<String, ?>> entityContentPredicate);


This cannot return Entity, since what this will return is not a valid entity. There needs to be a new type for this (maybe call it InvalidEntity), which gives information on why this is not a valid entity and provides some kind of information that might help in fixing the entity. For the former, I would recommend an error message and/or stack trace. For the latter, I would recommend a string containing the actual source code.

It's also not clear whether classifierPathPredicate or entityContentPredicate really make sense here. Since we can't assume that the entity can be deserialized, we can't be sure we'll be able to test those predicates.

Finally, there should be an API to get a single invalid entity by its path.

kevin-m-knight-gs · 2022-10-05T18:17:12Z

legend-sdlc-server/src/main/java/org/finos/legend/sdlc/server/gitlab/api/GitLabEntityApi.java

@@ -923,5 +939,18 @@ synchronized Entity getEntity()
            }
            return this.entity;
        }
+
+        synchronized Entity getInvalidEntity()


As with EntityAccessContext, this cannot return Entity. If the entity is indeed invalid, you should cache that fact and check it both in this method and in getEntity.

Also, this needs to check for any kind of invalidity, not just a path mismatch. If an entity cannot be deserialized for any reason, it is invalid.

...er/src/main/java/org/finos/legend/sdlc/server/resources/BackupWorkspaceEntitiesResource.java

kevin-m-knight-gs · 2022-10-25T13:54:41Z

...server/src/main/java/org/finos/legend/sdlc/server/domain/api/entity/EntityAccessContext.java

@@ -31,5 +32,9 @@ default List<Entity> getEntities(Predicate<String> entityPathPredicate, Predicat

    List<Entity> getEntities(Predicate<String> entityPathPredicate, Predicate<String> classifierPathPredicate, Predicate<? super Map<String, ?>> entityContentPredicate, boolean excludeInvalid);

+    InvalidEntity getInvalidEntity(String path);
+
+    List<InvalidEntity> getInvalidEntities();


Let's add a path predicate to getInvalidEntities so that they can be filtered. Also, let's add a method to get just the paths of invalid entities.

Suggested change

List<InvalidEntity> getInvalidEntities();

List<InvalidEntity> getInvalidEntities(Predicate<String> entityPathPredicate);

List<String> getInvalidEntityPaths(Predicate<String> entityPathPredicate);

kevin-m-knight-gs · 2022-10-25T14:04:09Z

legend-sdlc-model/src/main/java/org/finos/legend/sdlc/domain/model/entity/InvalidEntity.java

+
+    String getErrorMessage();
+
+    String getStackTrace();


Let's call this getErrorDetail instead of getStackTrace. That makes it a bit more generic, in case we have something other than a stack trace that we'd want to include.

Suggested change

String getStackTrace();

String getErrorDetail();

kevin-m-knight-gs · 2022-10-25T14:09:24Z

legend-sdlc-server/src/main/java/org/finos/legend/sdlc/server/gitlab/api/GitLabEntityApi.java

+                    {
+                        return InvalidEntity.newInvalidEntity(path, e.getMessage(), e.getStackTrace().toString(), file.getContentAsString());
+                    }
+                }


If we find the entity and it is valid, then we should throw an exception that says that the entity is valid. I'm not sure what the status code should be for this case. 404 doesn't seem quite right, since we did actually find the entity. But I can't think of something better. Maybe 409?

kevin-m-knight-gs · 2022-10-25T14:11:55Z

legend-sdlc-server/src/main/java/org/finos/legend/sdlc/server/gitlab/api/GitLabEntityApi.java

+                    }
+                }
+            }
+            throw new LegendSDLCServerException("Unknown invalid entity " + path + " for " + getInfoForException(), Status.NOT_FOUND);


This exception should only occur if we don't find any file at all for the entity.

Suggested change

throw new LegendSDLCServerException("Unknown invalid entity " + path + " for " + getInfoForException(), Status.NOT_FOUND);

throw new LegendSDLCServerException("Unknown entity " + path + " for " + getInfoForException(), Status.NOT_FOUND);

kevin-m-knight-gs · 2022-10-25T14:16:51Z

legend-sdlc-server/src/main/java/org/finos/legend/sdlc/server/gitlab/api/GitLabEntityApi.java

+            if (this.entity == null)
+            {
+                try
+                {
+                    Entity localEntity = this.sourceDirectory.deserialize(this.file);
+                    if (!Objects.equals(localEntity.getPath(), getEntityPath()))
+                    {
+                        throw new RuntimeException("Expected entity path " + getEntityPath() + ", found " + localEntity.getPath());
+                    }
+                }
+                catch (Exception e)
+                {
+                    return InvalidEntity.newInvalidEntity(getEntityPath(), e.getMessage(), e.getStackTrace().toString(), this.file.getContentAsString());
+                }
+            }


Suggested change

if (this.entity == null)

{

try

{

Entity localEntity = this.sourceDirectory.deserialize(this.file);

if (!Objects.equals(localEntity.getPath(), getEntityPath()))

{

throw new RuntimeException("Expected entity path " + getEntityPath() + ", found " + localEntity.getPath());

}

}

catch (Exception e)

{

return InvalidEntity.newInvalidEntity(getEntityPath(), e.getMessage(), e.getStackTrace().toString(), this.file.getContentAsString());

}

}

try

{

getEntity();

}

catch (Exception e)

{

return InvalidEntity.newInvalidEntity(getEntityPath(), e.getMessage(), e.getStackTrace().toString(), this.file.getContentAsString());

}

YannanGao-gs requested review from pierredebelen, kevin-m-knight-gs, akphi, MauricioUyaguari, davidharte-gs, hardikmaheshwari and rafaelbey as code owners October 4, 2022 20:45

finos-cla-bot bot added the cla-present label Oct 4, 2022

YannanGao-gs marked this pull request as draft October 4, 2022 20:45

YannanGao-gs force-pushed the get_invalid_entities branch from 5700899 to a87cec7 Compare October 5, 2022 15:29

YannanGao-gs changed the title ~~Add a new API for pulling invalid entities~~ A comprehensive error handling strategy of pulling entities -- part II Oct 5, 2022

kevin-m-knight-gs requested changes Oct 5, 2022

View reviewed changes

YannanGao-gs added the enhancement New feature or request label Oct 5, 2022

YannanGao-gs self-assigned this Oct 5, 2022

YannanGao-gs force-pushed the get_invalid_entities branch 5 times, most recently from 5354817 to 39068b4 Compare October 11, 2022 18:42

kevin-m-knight-gs requested changes Oct 25, 2022

View reviewed changes

A comprehensive error handling strategy of pulling entities -- part II

184c8b1

YannanGao-gs force-pushed the get_invalid_entities branch from 39068b4 to 184c8b1 Compare August 22, 2023 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A comprehensive error handling strategy of pulling entities -- part II #528

A comprehensive error handling strategy of pulling entities -- part II #528

YannanGao-gs commented Oct 4, 2022 •

edited

Loading

github-actions bot commented Oct 4, 2022 •

edited

Loading

kevin-m-knight-gs left a comment

kevin-m-knight-gs Oct 5, 2022

kevin-m-knight-gs Oct 5, 2022

kevin-m-knight-gs Oct 25, 2022

kevin-m-knight-gs Oct 25, 2022

kevin-m-knight-gs Oct 25, 2022

kevin-m-knight-gs Oct 25, 2022

kevin-m-knight-gs Oct 25, 2022

		@@ -26,5 +26,7 @@

		List<Entity> getEntities(Predicate<String> entityPathPredicate, Predicate<String> classifierPathPredicate, Predicate<? super Map<String, ?>> entityContentPredicate);

		List<Entity> getInvalidEntities(Predicate<String> entityPathPredicate, Predicate<String> classifierPathPredicate, Predicate<? super Map<String, ?>> entityContentPredicate);

-    List<InvalidEntity> getInvalidEntities();
+    List<InvalidEntity> getInvalidEntities(Predicate<String> entityPathPredicate);
+    List<String> getInvalidEntityPaths(Predicate<String> entityPathPredicate);

	throw new LegendSDLCServerException("Unknown invalid entity " + path + " for " + getInfoForException(), Status.NOT_FOUND);
	throw new LegendSDLCServerException("Unknown entity " + path + " for " + getInfoForException(), Status.NOT_FOUND);

A comprehensive error handling strategy of pulling entities -- part II #528

Are you sure you want to change the base?

A comprehensive error handling strategy of pulling entities -- part II #528

Conversation

YannanGao-gs commented Oct 4, 2022 • edited Loading

github-actions bot commented Oct 4, 2022 • edited Loading

Test Results

kevin-m-knight-gs left a comment

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 5, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 5, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 25, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 25, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 25, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 25, 2022

Choose a reason for hiding this comment

kevin-m-knight-gs Oct 25, 2022

Choose a reason for hiding this comment

YannanGao-gs commented Oct 4, 2022 •

edited

Loading

github-actions bot commented Oct 4, 2022 •

edited

Loading