[NAE-2184] Nullable object as serializable alternative to Optional #346
Conversation
- Add Nullable utility class to handle nullable arguments in Plugins
- Added Nullable to action imports
WalkthroughAdds a new Nullable utility class, exposes it to Groovy actions via configuration and GroovyShell default imports, and adds a nullable(T) helper method in ActionDelegate. No existing control-flow semantics were altered. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Config as GroovyShellConfiguration
participant Imports as ImportCustomizer
participant YAML as application.yaml
participant Script as Groovy Action Script
participant Delegate as ActionDelegate
participant Nullable as Nullable<T>
Note over Config, YAML: Startup/configuration
Config->>Imports: addStarImports(getDefaultEngineImports())
Config->>Imports: addImports(from YAML.actions.imports)
YAML->>Config: actions.imports includes Nullable
Note over Script, Imports: Script execution context
Imports-->>Script: provide star-imports & imports (including Nullable)
Script->>Delegate: calls nullable(value)
Delegate->>Nullable: Nullable.of(value)
Nullable-->>Delegate: Nullable<T> instance
Delegate-->>Script: returns Nullable<T>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- Added serialVersionUID
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java (7)
60-64: Avoid shadowing constant in empty()Local variable
EMPTYshadows the class constant and hurts readability. Return the cast directly.- public static <T> Nullable<T> empty() { - @SuppressWarnings("unchecked") - Nullable<T> EMPTY = (Nullable<T>) Nullable.EMPTY; - return EMPTY; - } + public static <T> Nullable<T> empty() { + @SuppressWarnings("unchecked") + return (Nullable<T>) EMPTY; + }
89-93: Enforce non-null consumer in ifPresentThe Javadoc promises non-null; enforce it to fail fast and match style used elsewhere.
- public void ifPresent(Consumer<? super T> action) { - if (value != null) { - action.accept(value); - } - } + public void ifPresent(Consumer<? super T> action) { + Objects.requireNonNull(action, "action"); + if (value != null) { + action.accept(value); + } + }
101-107: Validate both parameters in ifPresentOrElseAdd null checks for
actionandemptyActionto align with the contract.- public void ifPresentOrElse(Consumer<? super T> action, Runnable emptyAction) { - if (value != null) { - action.accept(value); - } else { - emptyAction.run(); - } - } + public void ifPresentOrElse(Consumer<? super T> action, Runnable emptyAction) { + Objects.requireNonNull(action, "action"); + Objects.requireNonNull(emptyAction, "emptyAction"); + if (value != null) { + action.accept(value); + } else { + emptyAction.run(); + } + }
231-233: Guard supplier in orElseGet
Objects.requireNonNull(supplier)prevents NPEs and documents intent.- public T orElseGet(Supplier<? extends T> supplier) { - return value != null ? value : supplier.get(); - } + public T orElseGet(Supplier<? extends T> supplier) { + Objects.requireNonNull(supplier, "supplier"); + return value != null ? value : supplier.get(); + }
259-265: Guard exception supplier in orElseThrow(Supplier)Validate the
exceptionSupplierto avoid surprising NPEs.- public <X extends Throwable> T orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { - if (value != null) { - return value; - } else { - throw exceptionSupplier.get(); - } - } + public <X extends Throwable> T orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { + Objects.requireNonNull(exceptionSupplier, "exceptionSupplier"); + if (value != null) { + return value; + } else { + throw exceptionSupplier.get(); + } + }
296-296: Canonicalize empty on deserialization (optional)If you care about a single empty instance post-serialization, add
readResolve()to return the canonical empty.@@ @Override public int hashCode() { return Objects.hashCode(value); } + private Object readResolve() { + return isEmpty() ? Nullable.empty() : this; + } + /** * Returns a string representation of the object.
15-21: Unit tests request: core API and serializationPlease add focused tests: presence checks, map/flatMap/or semantics, both orElseThrow variants, equals/hashCode, stream(), and Java serialization round-trip (including empty canonicalization if added).
Also applies to: 89-107, 127-152, 164-195, 203-209, 217-247, 259-265
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
93-93: Drop the trailing semicolon; optional style cleanupGroovy imports don’t require semicolons; align with surrounding imports.
-import com.netgrif.application.engine.objects.utils.Nullable; +import com.netgrif.application.engine.objects.utils.Nullable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(1 hunks)application-engine/src/main/resources/application.yaml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java (1)
15-21: Clarify intended serialization channel (Java vs JSON)If Plugin exchange uses JSON (Jackson), this class won’t round-trip by default (no bean-style getter/ctor). Consider a separate Jackson adapter or annotations (e.g., @jsonvalue on value exposure and a @JsonCreator factory) in a jackson-dedicated module to avoid adding compile deps here. Confirm the channel.
application-engine/src/main/resources/application.yaml (1)
162-162: Registered Nullable for Actions DSL — LGTMImport addition looks correct and non-invasive.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
93-93: Confirm import necessity
Nullableisn’t referenced in this class. If DSL availability is handled solely viaactions.imports, this import can be removed to avoid unused-import warnings.
- Added default engine imports in `GroovyShellConfiguration` for better functionality. - Introduced a generic `nullable` method in `ActionDelegate` to handle optional values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java (1)
38-45: Minor: document intent and narrow surface for future readers.Consider adding a brief Javadoc explaining why these defaults are included and that subpackages aren’t covered (e.g., utils). This helps maintainers avoid “why isn’t X visible?” confusion.
Apply within this region:
@@ - protected String[] getDefaultEngineImports() { + /** + * Default Groovy star-imports available to action scripts. + * Note: Applies only to the listed packages (not their subpackages). + */ + protected String[] getDefaultEngineImports() { return new String[]{ "com.netgrif.application.engine.objects", "com.netgrif.application.engine.adapter.spring", "java.time" }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java (2)
11-12: No behavior change here.This import doesn’t affect runtime or bean wiring in this class. If it’s intentional for upcoming edits, keep it; otherwise your linter may suggest removing it.
23-27: Adding default star-imports is fine; please confirm no ambiguity is introduced.Star-importing additional packages can surface ambiguous simple names in Groovy scripts if another star-import provides a class with the same simple name. Your explicit imports (actionsProperties.getImports) still win, but unqualified references could now fail resolution. Verify scripts compile cleanly across modules after this change. If collisions appear, prefer explicit imports or fully qualified names in actions.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Outdated
Show resolved
Hide resolved
- Removed unnecessary static EMPTY field to simplify code and avoid redundancy. - Simplified `toString` method to return an empty string when no value is present.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java (6)
24-26: Use a singleton EMPTY to avoid per-call allocations.Optional-style reuse reduces GC churn while keeping equals/hashCode semantics.
@Serial private static final long serialVersionUID = 8683452581122892189L; + private static final Nullable<?> EMPTY = new Nullable<>(null); @@ - public static <T> Nullable<T> empty() { - return new Nullable<>(null); - } + @SuppressWarnings("unchecked") + public static <T> Nullable<T> empty() { + return (Nullable<T>) EMPTY; + }Also applies to: 58-60
182-191: Preserve laziness for suppliers and add explicit null checks only when needed.Avoid failing when value is present; validate inputs only on the executed branch.
- public Nullable<T> or(Supplier<? extends Nullable<? extends T>> supplier) { - Objects.requireNonNull(supplier); - if (isPresent()) { - return this; - } else { - @SuppressWarnings("unchecked") - Nullable<T> r = (Nullable<T>) supplier.get(); - return Objects.requireNonNull(r); - } - } + public Nullable<T> or(Supplier<? extends Nullable<? extends T>> supplier) { + if (isPresent()) { + return this; + } + Objects.requireNonNull(supplier, "supplier must not be null"); + @SuppressWarnings("unchecked") + Nullable<T> r = (Nullable<T>) supplier.get(); + return Objects.requireNonNull(r, "supplier.get() returned null"); + } @@ - public T orElseGet(Supplier<? extends T> supplier) { - return value != null ? value : supplier.get(); - } + public T orElseGet(Supplier<? extends T> supplier) { + if (value != null) return value; + Objects.requireNonNull(supplier, "supplier must not be null"); + return supplier.get(); + } @@ - public <X extends Throwable> T orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { - if (value != null) { - return value; - } else { - throw exceptionSupplier.get(); - } - } + public <X extends Throwable> T orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { + if (value != null) return value; + Objects.requireNonNull(exceptionSupplier, "exceptionSupplier must not be null"); + throw exceptionSupplier.get(); + }Also applies to: 227-229, 255-261
85-89: Branch-specific null checks for callbacks.Validate only the callback that is actually used; improves diagnostics while preserving laziness.
- if (value != null) { - action.accept(value); - } + if (value != null) { + Objects.requireNonNull(action, "action must not be null").accept(value); + } @@ - if (value != null) { - action.accept(value); - } else { - emptyAction.run(); - } + if (value != null) { + Objects.requireNonNull(action, "action must not be null").accept(value); + } else { + Objects.requireNonNull(emptyAction, "emptyAction must not be null").run(); + }Also applies to: 97-103
199-205: Minor: simplify stream() implementation.- if (isEmpty()) { - return Stream.empty(); - } else { - return Stream.of(value); - } + return isEmpty() ? Stream.empty() : Stream.of(value);
301-303: Make toString() informative and align with Javadoc.Empty string is ambiguous; prefer Optional-like format.
- return value != null ? value.toString() : ""; + return value != null ? "Nullable[" + value + "]" : "Nullable.empty";
22-31: Add tests for JSON round-trip and supplier laziness.Two quick unit tests will de-risk usage in Groovy/Plugin exchange.
I can add:
- Jackson round-trip: serialize Nullable.of("x") -> "x"; deserialize -> Nullable.of("x"); and Nullable.empty() -> null payload round-trip.
- Laziness:
nullable("v").or(null)doesn’t throw;nullable(null).or(() -> null)throws NPE with message.
Do you want me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java (1)
273-291: equals/hashCode: LGTM.Value-based equality and Objects.hashCode are correct.
| /** | ||
| * Returns the value held by this instance. | ||
| * | ||
| * @return the value contained in this instance, or {@code null} if the instance is empty | ||
| */ | ||
| public T get() { | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add Jackson annotations to guarantee JSON round-trip (aligns with PR goal).
Without a bean-style getter (e.g., getValue) Jackson will likely serialize this as {}. Expose the wrapped value via @jsonvalue and enable delegating construction via @JsonCreator.
Apply:
import java.io.Serial;
import java.io.Serializable;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonValue;
@@
- public T get() {
+ @JsonValue
+ public T get() {
return value;
}
@@
- public static <T> Nullable<T> of(T value) {
+ @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
+ public static <T> Nullable<T> of(T value) {
return new Nullable<>(value);
}Also applies to: 48-51
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
around lines 33-41 (and also apply to lines 48-51), the wrapped value isn't
exposed for Jackson so instances serialize as {} — annotate the getter with
@JsonValue to serialize the contained value, and add @JsonCreator (use
Mode.DELEGATING) to the constructor or factory so Jackson can deserialize
directly into the wrapper; add the necessary imports for
com.fasterxml.jackson.annotation.JsonValue and
com.fasterxml.jackson.annotation.JsonCreator and apply the annotations to the
existing get() and constructor/factory accordingly.
Description
Creation of Nullable class to serialize nullable values in Plugin exchange
NAE-2184
Dependencies
No new Dependencies introduced.
Third party dependencies
No new Third Party Dependencies introduced.
Blocking Pull requests
None
How Has Been This Tested?
User tested in cluster environment with docker.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Configuration
Stability
Chores