add isAbstract to templates and do validation on update/create#164
add isAbstract to templates and do validation on update/create#164
Conversation
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant TemplateService
participant Repository
Client->>Service: createEntity(request with templateId)
Service->>Repository: loadTemplate(templateId)
Repository-->>Service: template
Service->>TemplateService: validateTemplateUsage(template)
alt template is abstract or deprecated
TemplateService-->>Service: throw IllegalStateException
Service-->>Client: error response
else valid template
TemplateService-->>Service: validation ok
Service->>Repository: save entity with template
Repository-->>Service: success
Service-->>Client: success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sync-github/src/main/kotlin/gropius/sync/github/config/IMSConfigManager.kt (1)
152-182: Exclude abstract templates from selection sets.
findTemplates()only filters!isDeprecated; with the newisAbstractflag, an abstract template can satisfy the “identical” check and block creation of a usable template, or be returned for use and later rejected by validation. Filter outisAbstractin both template queries.🔧 Suggested fix
- val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { - (!it.isDeprecated) && isContentCompatible( + val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { + (!it.isDeprecated) && (!it.isAbstract) && isContentCompatible( it, IMSConfig.IMS_TEMPLATE_NAME, IMSConfig.IMS_TEMPLATE_FIELDS ) && isContentCompatible( it.imsProjectTemplate().value, IMSProjectConfig.IMS_PROJECT_TEMPLATE_NAME, IMSProjectConfig.IMS_PROJECT_TEMPLATE_FIELDS ) && isContentCompatible( it.imsIssueTemplate().value, IMS_ISSUE_TEMPLATE_NAME, IMS_ISSUE_TEMPLATE_FIELDS ) && isContentCompatible( it.imsUserTemplate().value, IMS_USER_TEMPLATE_NAME, IMS_USER_TEMPLATE_FIELDS ) }.toSet().toMutableSet() val identicalTemplates = neoOperations.findAll<IMSTemplate>().filter { - (!it.isDeprecated) && isContentIdentical( + (!it.isDeprecated) && (!it.isAbstract) && isContentIdentical( it, IMSConfig.IMS_TEMPLATE_NAME, IMSConfig.IMS_TEMPLATE_FIELDS ) && isContentIdentical( it.imsProjectTemplate().value, IMSProjectConfig.IMS_PROJECT_TEMPLATE_NAME, IMSProjectConfig.IMS_PROJECT_TEMPLATE_FIELDS ) && isContentIdentical( it.imsIssueTemplate().value, IMS_ISSUE_TEMPLATE_NAME, IMS_ISSUE_TEMPLATE_FIELDS ) && isContentIdentical( it.imsUserTemplate().value, IMS_USER_TEMPLATE_NAME, IMS_USER_TEMPLATE_FIELDS ) }.toSet().toMutableSet()sync-jira/src/main/kotlin/gropius/sync/jira/config/IMSConfigManager.kt (1)
112-128: Exclude abstract templates from the acceptable set.With the new
isAbstractflag,findTemplates()can currently accept abstract templates, which will later be rejected byvalidateTemplateUsageand break sync flows.🔧 Proposed fix
- val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { - (!it.isDeprecated) && isContentCompatible( + val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { + (!it.isDeprecated) && (!it.isAbstract) && isContentCompatible( it, IMSConfig.IMS_TEMPLATE_NAME, IMSConfig.IMS_TEMPLATE_FIELDS ) && isContentCompatible( it.imsProjectTemplate().value, IMSProjectConfig.IMS_PROJECT_TEMPLATE_NAME, IMSProjectConfig.IMS_PROJECT_TEMPLATE_FIELDS ) && isContentCompatible( it.imsIssueTemplate().value, IMS_ISSUE_TEMPLATE_NAME, IMS_ISSUE_TEMPLATE_FIELDS ) && isContentCompatible( it.imsUserTemplate().value, IMS_USER_TEMPLATE_NAME, IMS_USER_TEMPLATE_FIELDS ) }.toSet().toMutableSet()
🧹 Nitpick comments (4)
core/src/main/kotlin/gropius/service/template/TemplateService.kt (1)
28-35: Consider enhancing error messages with template identification.The error messages are generic and don't identify which template failed validation. Including the template's name or ID would improve debuggability.
♻️ Suggested improvement
suspend fun validateTemplateUsage(template: Template<*, *>) { if (template.isAbstract) { - throw IllegalStateException("Cannot use abstract template") + throw IllegalStateException("Cannot use abstract template: ${template.name}") } if (template.isDeprecated) { - throw IllegalStateException("Cannot use deprecated template") + throw IllegalStateException("Cannot use deprecated template: ${template.name}") } }core/src/main/kotlin/gropius/service/architecture/InterfaceSpecificationService.kt (1)
86-91: Consider validating template usage earlier for fail-fast behavior.The template validation at line 90 occurs after
validateInitialTemplatedFields(line 88) and after creating theInterfaceSpecificationobject (line 89). If the template is abstract or deprecated, this work is wasted. Moving validation immediately after fetching the template would fail faster.♻️ Suggested reordering
suspend fun createInterfaceSpecification( component: Component, input: InterfaceSpecificationInput ): InterfaceSpecification { input.validate() val template = interfaceSpecificationTemplateRepository.findById(input.template) + templateService.validateTemplateUsage(template) val templatedFields = templatedNodeService.validateInitialTemplatedFields(template, input) val interfaceSpecification = InterfaceSpecification(input.name, input.description, templatedFields) - templateService.validateTemplateUsage(template) interfaceSpecification.template().value = templatesync-jira/src/main/kotlin/gropius/sync/jira/JiraDataService.kt (1)
106-108: Consider using named arguments for Boolean parameters to improve readability.With multiple consecutive Boolean arguments, the code becomes harder to read and maintain. Named arguments would clarify the intent.
♻️ Suggested improvement
return neoOperations.findAll(IssueTemplate::class.java).awaitFirstOrNull() ?: neoOperations.save( - IssueTemplate("noissue", "", mutableMapOf(), false, false) + IssueTemplate( + name = "noissue", + description = "", + templateFieldSpecifications = mutableMapOf(), + isDeprecated = false, + isAbstract = false + ) ).awaitSingle()core/src/main/kotlin/gropius/service/architecture/IMSService.kt (1)
56-60: Consider validating template usage earlier for fail-fast behavior.Similar to the pattern in
InterfaceSpecificationService, the template validation occurs after creating theIMSobject and validating templated fields. Moving validation immediately after fetching the template would avoid unnecessary work if the template is invalid.♻️ Suggested reordering
val template = imsTemplateRepository.findById(input.template) + templateService.validateTemplateUsage(template) val templatedFields = templatedNodeService.validateInitialTemplatedFields(template, input) val ims = IMS(input.name, input.description, templatedFields) - templateService.validateTemplateUsage(template) ims.template().value = template
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sync-github/src/main/kotlin/gropius/sync/github/config/IMSConfigManager.kt (1)
153-178: Also exclude abstract/deprecated sub-templates.Line 153 and Line 167 only filter
IMSTemplateitself. If any of the nestedIMSProjectTemplate,IMSIssueTemplate, orIMSUserTemplateare abstract/deprecated, they can still be selected and assigned, which undermines the new validation intent.💡 Suggested fix
- val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { - (!it.isDeprecated && !it.isAbstract) && isContentCompatible( + val acceptableTemplates = neoOperations.findAll<IMSTemplate>().filter { + (!it.isDeprecated && !it.isAbstract) + && (!it.imsProjectTemplate().value.isDeprecated && !it.imsProjectTemplate().value.isAbstract) + && (!it.imsIssueTemplate().value.isDeprecated && !it.imsIssueTemplate().value.isAbstract) + && (!it.imsUserTemplate().value.isDeprecated && !it.imsUserTemplate().value.isAbstract) + && isContentCompatible( it, IMSConfig.IMS_TEMPLATE_NAME, IMSConfig.IMS_TEMPLATE_FIELDS ) && isContentCompatible( it.imsProjectTemplate().value, IMSProjectConfig.IMS_PROJECT_TEMPLATE_NAME, IMSProjectConfig.IMS_PROJECT_TEMPLATE_FIELDS ) && isContentCompatible( it.imsIssueTemplate().value, IMS_ISSUE_TEMPLATE_NAME, IMS_ISSUE_TEMPLATE_FIELDS ) && isContentCompatible( it.imsUserTemplate().value, IMS_USER_TEMPLATE_NAME, IMS_USER_TEMPLATE_FIELDS ) }.toSet().toMutableSet() val identicalTemplates = neoOperations.findAll<IMSTemplate>().filter { - (!it.isDeprecated && !it.isAbstract) && isContentIdentical( + (!it.isDeprecated && !it.isAbstract) + && (!it.imsProjectTemplate().value.isDeprecated && !it.imsProjectTemplate().value.isAbstract) + && (!it.imsIssueTemplate().value.isDeprecated && !it.imsIssueTemplate().value.isAbstract) + && (!it.imsUserTemplate().value.isDeprecated && !it.imsUserTemplate().value.isAbstract) + && isContentIdentical( it, IMSConfig.IMS_TEMPLATE_NAME, IMSConfig.IMS_TEMPLATE_FIELDS ) && isContentIdentical( it.imsProjectTemplate().value, IMSProjectConfig.IMS_PROJECT_TEMPLATE_NAME, IMSProjectConfig.IMS_PROJECT_TEMPLATE_FIELDS ) && isContentIdentical( it.imsIssueTemplate().value, IMS_ISSUE_TEMPLATE_NAME, IMS_ISSUE_TEMPLATE_FIELDS ) && isContentIdentical( it.imsUserTemplate().value, IMS_USER_TEMPLATE_NAME, IMS_USER_TEMPLATE_FIELDS ) }.toSet().toMutableSet()
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.