Create a new piezo endpoint for updating data by jdforde · Pull Request #136 · lucidsoftware/piezo

jdforde · 2026-09-03T17:47:31Z

Create a new piezo endpoint that allows us to update job data for multiple jobs at once

ifndefJOSH

I just have a couple of questions about whether or not there's any potentially sensitive information about jobs in error messages that could leak out?

ifndefJOSH · 2026-09-03T19:33:04Z

+        val errorMsg = "Exception caught updating the job data map of job %s %s. -- %s".format(
+          jobKey.getGroup,
+          jobKey.getName,
+          e.getLocalizedMessage(),


Is this exception passed through to the HTTP response? If so we need to verify nothing sensitive is exposed.

ifndefJOSH · 2026-09-03T19:34:49Z

+          "updated" -> updated.map(jobKeyJson),
+          "failures" -> failures,
+        )
+        if (failures.isEmpty) Ok(body) else MultiStatus(body)


I'm curious who will be calling this endpoint? Other services or will it be public? If the latter, failures need to be sanitized to ensure no sensitive data leaks out.

The intention is another service. but piezo doesn't really have a way to distinguish

tate8 · 2026-09-03T22:06:14Z

 GET     /data/jobs                         com.lucidchart.piezo.admin.controllers.Jobs.getJobsDetail
 GET     /data/jobs/:group/:name            com.lucidchart.piezo.admin.controllers.Jobs.getJobDetail(group: String, name: String)

+POST    /data/jobs/job-data-map            com.lucidchart.piezo.admin.controllers.Jobs.patchJobDataMaps


Curious why this is a POST vs a PATCH request which the controller method implies.

I don't think it matters too much in this case.

tate8 · 2026-09-03T22:21:34Z

Could an in progress job persist its job data after this endpoint updates the lastRunTime thereby overwriting the reset? Does there need to be some mechanism to pause new instances of these jobs from starting and waiting for running executions? This is a rare race condition though but just double checking.

tmccombs · 2026-09-15T23:10:25Z

+   * The body looks like
+   * {{{
+   * {
+   *   "jobs": [


Do we anticipate having additional fields here? or would it make sense to just accept the array directly without the "jobs" field?

tmccombs · 2026-09-15T23:11:51Z

+   * Errors are collected across the whole `jobs` list, so a caller patching hundreds of jobs sees every bad entry at
+   * once rather than one per round trip.
+   */
+  def parse(json: JsValue): JsResult[List[JobDataMapPatch]] = {


nit: It might be a little cleaner if this was something like

private implicit val reads: Reads[JobDataMapPatch] = Reads(parseJob) // or inline parseJob private val listReads = (JsPath \ "jobs").read[List[JobDataMapPatch]] def parse(json: JsValue): JsResult[List[JobDataMapPatch]] = json.validate(listReads)

tmccombs · 2026-09-15T23:21:29Z

+
+  private def parseJob(json: JsValue): JsResult[JobDataMapPatch] = {
+    for {
+      group <- pathed((json \ "group").validate[String], JsPath \ "group")


Instead of using this pathed function you can get equivalent functionality with

group <- json.validate((JsPath \ "group").read[String])

or

group <- (JsPath \ "group").read[String].reads(json)

tmccombs · 2026-09-15T23:24:23Z

+    } yield JobDataMapPatch(new JobKey(name, group), entries)
+  }
+
+  private def parseEntries(dataMap: JsLookupResult): JsResult[List[DataMap]] = {


why accept an array or an object? this could be simpler if we only accept one, and we don't have to worry about backwards compatibility?

tmccombs · 2026-09-15T23:27:43Z

+    }
+  }
+
+  private def parsePairEntry(pair: JsValue): JsResult[DataMap] = {


If we want to keep this, we should make it a reads on thedefinition of Datamap

tmccombs · 2026-09-15T23:34:28Z

  }

+  def patchJobDataMaps: Action[JsValue] = Action(parse.json(maxFormSize)) { request =>
+    JobDataMapPatch.parse(request.body) match {


If we define a Reads for the jobdatamappatch, we can have play handle the parsing (and returning errors for invalid data) for us by using parse.json[List[JobDataMapPatch]] (if we just take an array) or parse.json(JobDataMapPatch.listReads) if we want to keep the wrapper object.

tmccombs · 2026-09-15T23:35:25Z

+          "updated" -> updated.map(jobKeyJson),
+          "failures" -> failures,
+        )
+        if (failures.isEmpty) Ok(body) else MultiStatus(body)


The intention is another service. but piezo doesn't really have a way to distinguish

tmccombs · 2026-09-15T23:36:12Z

+      case JsSuccess(patches, _) =>
+        val (failures, updated) = patches.map(applyJobDataMapPatch).partitionMap(identity)
+        val body = Json.obj(
+          "count" -> patches.size,


this seems unnecessary, shouldn't the caller already know what the count is?

tmccombs · 2026-09-15T23:38:15Z

 GET     /data/jobs                         com.lucidchart.piezo.admin.controllers.Jobs.getJobsDetail
 GET     /data/jobs/:group/:name            com.lucidchart.piezo.admin.controllers.Jobs.getJobDetail(group: String, name: String)

+POST    /data/jobs/job-data-map            com.lucidchart.piezo.admin.controllers.Jobs.patchJobDataMaps


I don't think it matters too much in this case.

tmccombs · 2026-09-15T23:41:17Z

+import scala.concurrent.Future
+import scala.jdk.CollectionConverters.*
+
+class JobDataMapService extends Specification {


why is this called a service?

This looks like a test, but it doesn't have Test or Spec in the name

Create a new piezo endpoint for updating data

bd8b46d

jdforde requested a review from tmccombs September 3, 2026 17:47

ifndefJOSH suggested changes Sep 3, 2026

View reviewed changes

tate8 reviewed Sep 3, 2026

View reviewed changes

tmccombs requested changes Sep 15, 2026

View reviewed changes

Uh oh!

Conversation

jdforde commented Sep 3, 2026

Uh oh!

ifndefJOSH 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!

tate8 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tate8 commented Sep 3, 2026 •

edited

Loading