Optional requiredParam for UIParameter decoder compatibility #7265
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the In the No significant changes to method signatures or the overall structure of the classes are present beyond those mentioned. Sequence Diagram(s)sequenceDiagram
participant Client
participant DefinitionsService
participant UIParameter
Client->>DefinitionsService: Request to create UIParameter
DefinitionsService->>UIParameter: Create UIParameter with Option[Boolean]
UIParameter-->>DefinitionsService: Return UIParameter instance
DefinitionsService-->>Client: Return created UIParameter
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)docs-internal/api/nu-designer-openapi.yaml (1)
Making Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/definition/package.scala (3)
Line range hint
38-56: Consider enhancing documentation about external project dependencyThe comment "This attribute is used only by external project" for
requiredParamcould be more descriptive. Consider documenting:
- Which external project depends on this
- Why it's needed
- Any plans for future removal or migration
This would help future maintainers understand the importance and lifecycle of this field.
73-75: Enhance version compatibility documentationThe comment about version 1.18 compatibility could be more specific. Consider:
- Adding the release date of version 1.18
- Specifying when this fallback might be removed (e.g., "can be removed after version X.Y")
- Adding a JIRA/GitHub issue reference for tracking the future removal
61-89: Consider using applicative syntax for a more concise decoderWhile the current implementation is correct, it could be more concise using Circe's applicative syntax. This would make the code more maintainable and reduce the likelihood of field mapping errors.
Here's a suggested refactoring:
implicit val uiParameterDecoder: Decoder[UIParameter] = { - Decoder.instance { c => - for { - name <- c.downField("name").as[String] - typ <- c.downField("typ").as[TypingResult] - editor <- c.downField("editor").as[ParameterEditor] - defaultValue <- c.downField("defaultValue").as[Expression] - additionalVariables <- c.downField("additionalVariables").as[Map[String, TypingResult]] - variablesToHide <- c.downField("variablesToHide").as[Set[String]] - branchParam <- c.downField("branchParam").as[Boolean] - hintText <- c.downField("hintText").as[Option[String]] - label <- c.downField("label").as[String] - requiredParam <- c.downField("requiredParam").as[Option[Boolean]].map(_.getOrElse(true)) - } yield UIParameter( - name = name, - typ = typ, - editor = editor, - defaultValue = defaultValue, - additionalVariables = additionalVariables, - variablesToHide = variablesToHide, - branchParam = branchParam, - hintText = hintText, - label = label, - requiredParam = requiredParam - ) - } + import io.circe.syntax._ + Decoder.instance { c => + ( + c.downField("name").as[String], + c.downField("typ").as[TypingResult], + c.downField("editor").as[ParameterEditor], + c.downField("defaultValue").as[Expression], + c.downField("additionalVariables").as[Map[String, TypingResult]], + c.downField("variablesToHide").as[Set[String]], + c.downField("branchParam").as[Boolean], + c.downField("hintText").as[Option[String]], + c.downField("label").as[String], + c.downField("requiredParam").as[Option[Boolean]].map(_.getOrElse(true)) + ).mapN(UIParameter.apply) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/definition/package.scala(3 hunks)
🔇 Additional comments (2)
designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/definition/package.scala (2)
4-4: LGTM: Import changes align with the new JSON handling approach
The import changes correctly reflect the transition from automatic codec derivation to manual decoder implementation while maintaining the configured encoder.
57-91: LGTM: Well-implemented JSON handling with backward compatibility
The implementation successfully:
- Provides a fallback for missing
requiredParam - Maintains backward compatibility with version 1.18
- Uses appropriate JSON encoding/decoding patterns
- Includes clear documentation
The code is well-structured and follows Scala best practices.
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
UIParameterclass, allowing therequiredParamfield to be optional.DefinitionsServiceclass.Bug Fixes
requiredParamfield.Documentation