Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a new API to update provided query fields and update query protocol to record originalVersionId #2633

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Introduce a new API to update provided query fields and update query protocol to record originalVersionId #2633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7eb9c95
Introduce a new API to update query version and update query protocol…
YannanGao-gs Feb 16, 2024
15b5824
clean code
YannanGao-gs Mar 4, 2024
2bb6236
rebase
YannanGao-gs Mar 4, 2024
932e62b
rename vairable
YannanGao-gs Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions ...n-query/src/main/java/org/finos/legend/engine/application/query/api/ApplicationQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,28 @@ public Response updateQuery(@PathParam("queryId") String queryId, Query query, @
}
}

@PUT
@Path("{queryId}/patchQuery")
@ApiOperation(value = "Patch Query - update selected query fields")
@Consumes({MediaType.APPLICATION_JSON})
public Response patchQuery(@PathParam("queryId") String queryId, Query query, @ApiParam(hidden = true) @Pac4JProfileManager ProfileManager<CommonProfile> profileManager)
{
MutableList<CommonProfile> profiles = ProfileManagerHelper.extractProfiles(profileManager);
Identity identity = IdentityFactoryProvider.getInstance().makeIdentity(profiles);
try (Scope scope = GlobalTracer.get().buildSpan("Patch Query - update selected query fields").startActive(true))
{
return Response.ok().entity(this.queryStoreManager.patchQuery(queryId, query, getCurrentUser(profileManager))).build();
}
catch (Exception e)
{
if (e instanceof ApplicationQueryException)
{
return ((ApplicationQueryException) e).toResponse();
}
return ExceptionTool.exceptionManager(e, LoggingEventType.UPDATE_QUERY_ERROR, identity.getName());
}
}

@DELETE
@Path("{queryId}")
@ApiOperation(value = "Delete the query with specified ID")
Expand Down
40 changes: 39 additions & 1 deletion ...-query/src/main/java/org/finos/legend/engine/application/query/api/QueryStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public class QueryStoreManager
// so that it records the dataSpace it is created from
private static final String QUERY_PROFILE_PATH = "meta::pure::profiles::query";
private static final String QUERY_PROFILE_TAG_DATA_SPACE = "dataSpace";
private static final List<String> LIGHT_QUERY_PROJECTION = Arrays.asList("id", "name", "versionId", "groupId", "artifactId", "owner", "createdAt", "lastUpdatedAt");
private static final List<String> LIGHT_QUERY_PROJECTION = Arrays.asList("id", "name", "versionId", "originalVersionId", "groupId", "artifactId", "owner", "createdAt", "lastUpdatedAt");
private static final int GET_QUERIES_LIMIT = 50;
private final MongoClient mongoClient;

Expand Down Expand Up @@ -110,6 +110,7 @@ private static Query documentToQuery(Document document)
query.groupId = document.getString("groupId");
query.artifactId = document.getString("artifactId");
query.versionId = document.getString("versionId");
query.originalVersionId = document.getString("originalVersionId");
query.mapping = document.getString("mapping");
query.runtime = document.getString("runtime");
query.content = document.getString("content");
Expand Down Expand Up @@ -386,13 +387,50 @@ else if (matchingQueries.size() == 0)
query.owner = currentUser;
query.createdAt = currentQuery.createdAt;
query.lastUpdatedAt = Instant.now().toEpochMilli();
query.originalVersionId = currentQuery.originalVersionId;
this.getQueryCollection().findOneAndReplace(Filters.eq("id", queryId), queryToDocument(query));
QueryEvent updatedEvent = createEvent(query.id, QueryEvent.QueryEventType.UPDATED);
updatedEvent.timestamp = query.lastUpdatedAt;
this.getQueryEventCollection().insertOne(queryEventToDocument(updatedEvent));
return query;
}

public Query patchQuery(String queryId, Query updatedQuery, String currentUser) throws JsonProcessingException
{
Query currentQuery = this.getQuery(queryId);
// Make sure only the owner can update the query
// NOTE: if the query is created by an anonymous user previously, set the current user as the owner
if (currentQuery.owner != null && !currentQuery.owner.equals(currentUser))
{
throw new ApplicationQueryException("Only owner can update the query", Response.Status.FORBIDDEN);
}

Class<? extends Query> queryClass = currentQuery.getClass();
for (java.lang.reflect.Field field : queryClass.getDeclaredFields())
{
try
{
field.setAccessible(true);
Object updatedValue = field.get(updatedQuery);
if (updatedValue != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might require some rethinking about if we have the api take from Query updatedQuery to something ese
but one use case this current flow does not support is if we want to set a value to null if it has already been set to something.
woud using the mongo set operation help here, and update only those fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the patchQuery function still takes in a parameter of type Query that we are going to update fields upon, to generate the payload of mongo set operation, I think we'll still need to compose a document by iterating the provided parameter to get non-null fields, then call set? 😅😅

Or, could you please be more specific about what the parameter type should be? 👀😅😅
Thanks

{
field.set(currentQuery, updatedValue);
}
}
catch (IllegalAccessException e)
{
throw new ApplicationQueryException("Can't modify query field" + field.getName(), Response.Status.BAD_REQUEST);
}
}
currentQuery.owner = currentUser;
currentQuery.lastUpdatedAt = Instant.now().toEpochMilli();
this.getQueryCollection().findOneAndReplace(Filters.eq("id", queryId), queryToDocument(currentQuery));
QueryEvent updatedEvent = createEvent(queryId, QueryEvent.QueryEventType.UPDATED);
updatedEvent.timestamp = currentQuery.lastUpdatedAt;
this.getQueryEventCollection().insertOne(queryEventToDocument(updatedEvent));
return currentQuery;
}

public void deleteQuery(String queryId, String currentUser) throws JsonProcessingException
{
List<Query> matchingQueries = LazyIterate.collect(this.getQueryCollection().find(Filters.eq("id", queryId)), QueryStoreManager::documentToQuery).toList();
Expand Down
1 change: 1 addition & 0 deletions ...pplication-query/src/main/java/org/finos/legend/engine/application/query/model/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class Query
public String groupId;
public String artifactId;
public String versionId;
public String originalVersionId;
public String mapping;
public String runtime;
public String content;
Expand Down
19 changes: 19 additions & 0 deletions ...ry/src/test/java/org/finos/legend/engine/application/query/api/TestQueryStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ static class TestQueryBuilder
public String groupId = "test.group";
public String artifactId = "test-artifact";
public String versionId = "0.0.0";
public String originalVersionId = "0.0.0";
public String description = "description";
public String mapping = "mapping";
public String runtime = "runtime";
Expand Down Expand Up @@ -187,6 +188,7 @@ Query build()
query.groupId = this.groupId;
query.artifactId = this.artifactId;
query.versionId = this.versionId;
query.originalVersionId = this.originalVersionId;
query.mapping = this.mapping;
query.runtime = this.runtime;
query.content = this.content;
Expand Down Expand Up @@ -353,6 +355,7 @@ public void testSearchQueries() throws Exception
Assert.assertEquals("1", lightQuery.id);
Assert.assertEquals("query1", lightQuery.name);
Assert.assertEquals("0.0.0", lightQuery.versionId);
Assert.assertEquals("0.0.0", lightQuery.originalVersionId);
Assert.assertNotNull(lightQuery.createdAt);
Assert.assertNotNull(lightQuery.lastUpdatedAt);
Assert.assertNull(lightQuery.content);
Expand Down Expand Up @@ -566,6 +569,7 @@ public void testCreateSimpleQuery() throws Exception
Assert.assertEquals("1", createdQuery.id);
Assert.assertEquals("query1", createdQuery.name);
Assert.assertEquals("0.0.0", createdQuery.versionId);
Assert.assertEquals("0.0.0", createdQuery.originalVersionId);
Assert.assertEquals("content", createdQuery.content);
Assert.assertEquals("description", createdQuery.description);
Assert.assertEquals("mapping", createdQuery.mapping);
Expand Down Expand Up @@ -613,6 +617,21 @@ public void testUpdateQuery() throws Exception
Assert.assertEquals("query2", queryStoreManager.getQuery("1").name);
}

@Test
public void testUpdateQueryVersion() throws Exception
{
String currentUser = "testUser";
queryStoreManager.createQuery(TestQueryBuilder.create("1", "query1", currentUser).build(), currentUser);
queryStoreManager.updateQuery("1", TestQueryBuilder.create("1", "query2", currentUser).build(), currentUser);
Assert.assertEquals("query2", queryStoreManager.getQuery("1").name);
Query queryWithSelectedFields = new Query();
queryWithSelectedFields.id = "1";
queryWithSelectedFields.versionId = "1.0.0";
queryStoreManager.patchQuery("1", queryWithSelectedFields, currentUser);
Assert.assertEquals("1.0.0", queryStoreManager.getQuery("1").versionId);
Assert.assertEquals("0.0.0", queryStoreManager.getQuery("1").originalVersionId);
}

@Test
public void testUpdateWithInvalidQuery()
{
Expand Down
Loading