Copilot commented on code in PR #12715:
URL: https://github.com/apache/gravitino/pull/12715#discussion_r3894476113
##########
docs/open-api/jobs.yaml:
##########
@@ -586,6 +586,10 @@ components:
format: date-time
nullable: true
description: The time when the job finished execution, or null if
the job has not finished execution yet
+ runtimeJobTemplate:
+ description: The resolved job template that was actually submitted
for execution, with placeholders replaced and referenced files downloaded.
Omitted for jobs run before this field was introduced
+ allOf:
+ - $ref: "#/components/schemas/JobTemplate"
Review Comment:
The API description states this field can be omitted for older jobs, but the
server-side behavior/tests also indicate it may be present as `null` (e.g.,
malformed stored JSON or missing historical data). To match actual responses,
mark `runtimeJobTemplate` as `nullable: true` (and keep it non-required).
Optionally replace `allOf` with a direct `$ref` unless you plan to extend it
with additional constraints.
##########
clients/client-python/gravitino/dto/job/job_template_dto.py:
##########
@@ -97,17 +97,25 @@ def validate(self) -> None:
raise ValueError('"executable" is required and cannot be empty')
@classmethod
- def from_json(
- cls, s: str, infer_missing: bool = False, **kwargs
+ def from_dict_by_type(
+ cls, data: Dict, infer_missing: bool = False
) -> "JobTemplateDTO":
- """Creates a JobTemplateDTO from a JSON string."""
- data = json.loads(s)
+ """Creates a JobTemplateDTO from a dict, dispatching to the concrete
subclass based
+ on the "jobType" field.
+ """
job_type = JobType.job_type_deserialize(data.get("jobType"))
subclass = JOB_TYPE_TEMPLATE_MAPPING.get(job_type)
if not subclass:
raise ValueError(f"Unsupported job type: {job_type}")
return subclass.from_dict(data, infer_missing=infer_missing)
+ @classmethod
+ def from_json(
+ cls, s: str, infer_missing: bool = False, **kwargs
+ ) -> "JobTemplateDTO":
+ """Creates a JobTemplateDTO from a JSON string."""
+ return cls.from_dict_by_type(json.loads(s),
infer_missing=infer_missing)
Review Comment:
`from_json(..., **kwargs)` now silently ignores `**kwargs`, which can be
confusing for callers and makes future changes harder (it looks like kwargs are
supported but they aren’t). Consider either removing `**kwargs` from the
signature, or explicitly documenting that kwargs are ignored; if backward
compatibility requires keeping it, pass through supported options where
applicable.
##########
common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java:
##########
@@ -1413,6 +1415,82 @@ public static JobTemplate fromDTO(JobTemplateDTO
jobTemplateDTO) {
}
}
+ /**
+ * Converts a JobTemplate to a JobTemplateDTO.
+ *
+ * @param jobTemplate The job template to be converted.
+ * @param audit The audit information to attach to the DTO. A bare {@link
JobTemplate} carries no
+ * audit info of its own, so the caller supplies it (e.g. the
originating template entity's
+ * audit info, when serializing a resolved runtime template).
+ * @return The job template DTO.
+ */
+ public static JobTemplateDTO toDTO(JobTemplate jobTemplate, AuditDTO audit) {
+ switch (jobTemplate.jobType()) {
+ case SHELL:
+ ShellJobTemplate shellJobTemplate = (ShellJobTemplate) jobTemplate;
+ return ShellJobTemplateDTO.builder()
+ .withName(shellJobTemplate.name())
+ .withComment(shellJobTemplate.comment())
+ .withJobType(shellJobTemplate.jobType())
+ .withExecutable(shellJobTemplate.executable())
+ .withArguments(shellJobTemplate.arguments())
+ .withEnvironments(shellJobTemplate.environments())
+ .withCustomFields(shellJobTemplate.customFields())
+ .withScripts(shellJobTemplate.scripts())
+ .withAudit(audit)
+ .build();
+
+ case SPARK:
+ SparkJobTemplate sparkJobTemplate = (SparkJobTemplate) jobTemplate;
+ return SparkJobTemplateDTO.builder()
+ .withName(sparkJobTemplate.name())
+ .withComment(sparkJobTemplate.comment())
+ .withJobType(sparkJobTemplate.jobType())
+ .withExecutable(sparkJobTemplate.executable())
+ .withArguments(sparkJobTemplate.arguments())
+ .withEnvironments(sparkJobTemplate.environments())
+ .withCustomFields(sparkJobTemplate.customFields())
+ .withClassName(sparkJobTemplate.className())
+ .withJars(sparkJobTemplate.jars())
+ .withFiles(sparkJobTemplate.files())
+ .withArchives(sparkJobTemplate.archives())
+ .withConfigs(sparkJobTemplate.configs())
+ .withAudit(audit)
+ .build();
+
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported job template type: " + jobTemplate.jobType());
+ }
+ }
+
+ /**
+ * Deserializes a job entity's stored runtime job template JSON, if any,
back into a {@link
+ * JobTemplateDTO}. {@link JobTemplateDTO}'s {@code @JsonTypeInfo} handles
the Shell/Spark
+ * dispatch automatically.
+ *
+ * @param runtimeJobTemplateJson The serialized runtime job template, or
null if the job has none.
+ * @param jobName The name of the job the template belongs to, used in the
error message on
+ * failure.
+ * @return The deserialized job template DTO, or null if
runtimeJobTemplateJson is null.
+ */
+ public static JobTemplateDTO fromRuntimeJobTemplateJson(
+ String runtimeJobTemplateJson, String jobName) {
+ if (runtimeJobTemplateJson == null) {
+ return null;
+ }
+
+ try {
+ return JsonUtils.anyFieldMapper().readValue(runtimeJobTemplateJson,
JobTemplateDTO.class);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(
+ String.format(
+ "Failed to deserialize the runtime job template for job %s, raw
content: %s",
+ jobName, runtimeJobTemplateJson),
+ e);
+ }
+ }
Review Comment:
The exception message includes the full raw runtime template JSON, which can
contain sensitive data (e.g., environment variables, custom fields, local
paths) and can also introduce log-injection risk if logged (newlines/control
chars). Prefer not embedding full raw content in exception messages; instead
include safe metadata (job id, content length, a hash, and/or a
truncated/sanitized prefix) and keep full content only behind an explicit debug
flag if needed.
--
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]
