aglinxinyuan commented on code in PR #4138:
URL: https://github.com/apache/texera/pull/4138#discussion_r2650033738
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -712,18 +712,31 @@ class WorkflowResource extends LazyLogging {
}
@GET
- @Path("/owner_user")
Review Comment:
This change doesn’t align with the goal of the PR. The endpoint should
return only the owner’s username, not the full owner information. The field can
remain owner_user or be renamed to owner_name, but no additional owner details
should be included.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -712,18 +712,31 @@ class WorkflowResource extends LazyLogging {
}
@GET
- @Path("/owner_user")
- def getOwnerUser(@QueryParam("wid") wid: Integer): User = {
+ @Path("/owner_info")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def getOwnerInfo(
+ @QueryParam("wid") wid: Integer,
+ @QueryParam("fields") fields: java.util.List[String] // e.g.
&fields=name&fields=...
+ ): User = {
+
+ val allowedFields = Map(
Review Comment:
Ditto — this isn’t correct. We shouldn’t give developers a choice when
there’s only one accepted behavior.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -712,18 +712,31 @@ class WorkflowResource extends LazyLogging {
}
@GET
- @Path("/owner_user")
- def getOwnerUser(@QueryParam("wid") wid: Integer): User = {
+ @Path("/owner_info")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def getOwnerInfo(
+ @QueryParam("wid") wid: Integer,
+ @QueryParam("fields") fields: java.util.List[String] // e.g.
&fields=name&fields=...
Review Comment:
Please avoid over-engineering unplanned features in a single PR. We do not
plan to allow this endpoint to return different fields based on user selection.
Introducing a dynamic endpoint increases complexity and the risk of injection
attacks. Each endpoint should have a single, well-defined responsibility.
For example, this endpoint should always return the username. If we also
need the email, we can either introduce a separate endpoint for email or update
this endpoint to always return both username and email. We should not use
options or flags to control endpoint behavior.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -712,18 +712,31 @@ class WorkflowResource extends LazyLogging {
}
@GET
- @Path("/owner_user")
- def getOwnerUser(@QueryParam("wid") wid: Integer): User = {
+ @Path("/owner_info")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def getOwnerInfo(
+ @QueryParam("wid") wid: Integer,
+ @QueryParam("fields") fields: java.util.List[String] // e.g.
&fields=name&fields=...
+ ): User = {
+
+ val allowedFields = Map(
+ "name" -> USER.NAME
+ )
+
+ val requestedFields =
+ Option(fields)
+ .map(_.asScala.toList)
+ .getOrElse(List("name"))
+ .map(_.trim.toLowerCase)
+ .filter(_.nonEmpty)
+ .distinct
+
+ val selectedFields = requestedFields.map { field =>
Review Comment:
Ditto. If there’s a plan to support additional fields, that should be done
in a separate PR. This PR should remain simple and only return the username.
##########
frontend/src/app/hub/component/workflow/detail/hub-workflow-detail.component.ts:
##########
@@ -98,10 +98,10 @@ export class HubWorkflowDetailComponent implements
AfterViewInit, OnDestroy, OnI
this.viewCount = count;
});
this.workflowPersistService
- .getOwnerUser(this.wid)
+ .getOwnerInfo(this.wid)
Review Comment:
Just getOwnerName
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
