Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class LineChartOpDesc extends PythonOperatorDescriptor {
var xLabel: EncodableString = ""

@JsonProperty(value = "lines", required = true)
var lines: util.List[LineConfig] = _
var lines: util.List[LineConfig] = new util.ArrayList[LineConfig]()

override def getOutputSchemas(
inputSchemas: Map[PortIdentity, Schema]
Expand All @@ -64,7 +64,9 @@ class LineChartOpDesc extends PythonOperatorDescriptor {
)

def createPlotlyFigure(): PythonTemplateBuilder = {
val linesPart = lines.asScala
val configuredLines = Option(lines).getOrElse(new util.ArrayList[LineConfig]())
assert(configuredLines.asScala.nonEmpty, "At least one line must be configured")
val linesPart = configuredLines.asScala
Comment on lines +67 to +69
Copy link
Copy Markdown
Contributor

@aglinxinyuan aglinxinyuan May 6, 2026

Choose a reason for hiding this comment

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

the default-value + null-guard + non-empty assertion is slightly belt-and-braces. Since lines is an external mutable field, the contract that matters is just "non-null and non-empty," which collapses to a single assertion at the top:

def createPlotlyFigure(): PythonTemplateBuilder = {
  assert(lines != null && !lines.isEmpty, "At least one line must be configured")
  val linesPart = lines.asScala.map { ... }

That avoids the unused empty-list allocation on the null path and the double .asScala wrap. The current form is correct — just a bit easier to read this way.

.map { lineConf =>
val colorPart = if (lineConf.color != "") {
pyb"line={'color':${lineConf.color}}, marker={'color':${lineConf.color}}, "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,22 +96,34 @@ class LineChartOpDescSpec extends AnyFlatSpec with Matchers {
code should not include "Y_LBL_SENT"
}

it should "raise NullPointerException when lines is left at its null default" in {
// Pin: `var lines: util.List[LineConfig] = _` defaults to null, and
// `createPlotlyFigure` calls `lines.asScala.map(...)` without a null
// check. Calling `generatePythonCode` on a default-constructed LineChart
// therefore throws NPE rather than rendering an empty chart or raising
// an AssertionError. Documenting so a future fix that null-guards lines
// breaks this spec deliberately.
it should "raise AssertionError when lines is left at its default (empty list)" in {
// `var lines: util.List[LineConfig]` defaults to an empty ArrayList.
// `createPlotlyFigure` asserts nonEmpty on lines before iterating, so a
// default-constructed LineChartOpDesc raises AssertionError with a
// descriptive message rather than proceeding with no traces.
val op = new LineChartOpDesc
assertThrows[NullPointerException](op.generatePythonCode())
val ex = intercept[AssertionError](op.generatePythonCode())
ex.getMessage should include("At least one line must be configured")
}

it should "render code with an empty lines list (no NPE, no assertion)" in {
it should "raise AssertionError when lines is explicitly set to an empty list" in {
// `createPlotlyFigure` guards against an empty lines list with an assertion,
// matching the fail-fast pattern used by sibling visualizers
// (HeatMapOpDesc, BarChartOpDesc).
val op = configured
op.lines = new util.ArrayList[LineConfig]()
Comment thread
Ma77Ball marked this conversation as resolved.
val code = op.generatePythonCode()
code should include("plotly")
code should include("fig = go.Figure()")
assertThrows[AssertionError](op.generatePythonCode())
}

it should "raise AssertionError rather than NullPointerException when lines is set to null" in {
// `lines` is a public mutable field; Jackson deserializing an explicit JSON
// null or a caller assigning null can set it back to null even after the
// non-null default is in place. `createPlotlyFigure` wraps `lines` in
// `Option(...).getOrElse(emptyList)` before asserting nonEmpty, so a null
// assignment produces the descriptive AssertionError rather than an NPE.
val op = configured
op.lines = null
val ex = intercept[AssertionError](op.generatePythonCode())
ex.getMessage should include("At least one line must be configured")
}
}
Loading