diff --git a/CHANGELOG.md b/CHANGELOG.md index 87e7b2c..e689138 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Upcoming Version / (WIP) Improvements: * Stabilized consul registration and health checks * [ODRC-24](https://openlmis.atlassian.net/browse/ODRC-24) Global header and translations implemented for reports +* [OLMIS-8231](https://openlmis.atlassian.net/browse/OLMIS-8231): Preserve Jasper template parameters order as declared in the .jrxml file. +* [OLMIS-8235](https://openlmis.atlassian.net/browse/OLMIS-8235): Add override query parameter to Jasper template upload to safely replace an existing template. 1.5.0 / 2025-11-27 ================== diff --git a/src/main/java/org/openlmis/report/domain/JasperTemplate.java b/src/main/java/org/openlmis/report/domain/JasperTemplate.java index 2a37777..c3cb8e2 100644 --- a/src/main/java/org/openlmis/report/domain/JasperTemplate.java +++ b/src/main/java/org/openlmis/report/domain/JasperTemplate.java @@ -30,6 +30,7 @@ import javax.persistence.ManyToMany; import javax.persistence.ManyToOne; import javax.persistence.OneToMany; +import javax.persistence.OrderBy; import javax.persistence.PrePersist; import javax.persistence.PreUpdate; import javax.persistence.Table; @@ -72,6 +73,7 @@ public class JasperTemplate extends BaseEntity { fetch = FetchType.EAGER, orphanRemoval = true) @Fetch(FetchMode.SELECT) + @OrderBy("displayOrder ASC NULLS LAST") private List templateParameters; @ManyToMany(fetch = FetchType.EAGER) diff --git a/src/main/java/org/openlmis/report/domain/JasperTemplateParameter.java b/src/main/java/org/openlmis/report/domain/JasperTemplateParameter.java index 6510c2f..975c680 100644 --- a/src/main/java/org/openlmis/report/domain/JasperTemplateParameter.java +++ b/src/main/java/org/openlmis/report/domain/JasperTemplateParameter.java @@ -124,6 +124,11 @@ public class JasperTemplateParameter extends BaseEntity { @Setter private Boolean required; + @Column + @Getter + @Setter + private Integer displayOrder; + /** * Create new instance of JasperTemplateParameter based on given {@link Importer}. * @@ -144,6 +149,7 @@ public static JasperTemplateParameter newInstance(Importer importer) { jasperTemplateParameter.setSelectProperty(importer.getSelectProperty()); jasperTemplateParameter.setDisplayProperty(importer.getDisplayProperty()); jasperTemplateParameter.setRequired(importer.getRequired()); + jasperTemplateParameter.setDisplayOrder(importer.getDisplayOrder()); jasperTemplateParameter.setOptions(importer.getOptions()); jasperTemplateParameter.setDependencies(importer.getDependencies() .stream() @@ -171,6 +177,7 @@ public void export(Exporter exporter) { exporter.setSelectProperty(selectProperty); exporter.setDisplayProperty(displayProperty); exporter.setRequired(required); + exporter.setDisplayOrder(displayOrder); exporter.setOptions(options); exporter.setDependencies(dependencies .stream() @@ -212,6 +219,8 @@ public interface Exporter { void setRequired(Boolean required); + void setDisplayOrder(Integer displayOrder); + void setOptions(List options); void setDependencies(List dependencies); @@ -242,6 +251,8 @@ public interface Importer { Boolean getRequired(); + Integer getDisplayOrder(); + List getOptions(); List getDependencies(); diff --git a/src/main/java/org/openlmis/report/dto/JasperTemplateParameterDto.java b/src/main/java/org/openlmis/report/dto/JasperTemplateParameterDto.java index 414fb1e..fe77463 100644 --- a/src/main/java/org/openlmis/report/dto/JasperTemplateParameterDto.java +++ b/src/main/java/org/openlmis/report/dto/JasperTemplateParameterDto.java @@ -39,6 +39,7 @@ public class JasperTemplateParameterDto implements JasperTemplateParameter.Expor private String displayProperty; private String description; private Boolean required; + private Integer displayOrder; private List options; private List dependencies; diff --git a/src/main/java/org/openlmis/report/service/JasperTemplateService.java b/src/main/java/org/openlmis/report/service/JasperTemplateService.java index 985d691..936c932 100644 --- a/src/main/java/org/openlmis/report/service/JasperTemplateService.java +++ b/src/main/java/org/openlmis/report/service/JasperTemplateService.java @@ -15,6 +15,7 @@ package org.openlmis.report.service; +import static org.apache.commons.lang3.BooleanUtils.isTrue; import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.openlmis.report.i18n.AuthorizationMessageKeys.ERROR_RIGHT_NOT_FOUND; @@ -100,17 +101,19 @@ public class JasperTemplateService { private ReportCategoryRepository reportCategoryRepository; /** - * Saves a template with given name. - * If template already exists, only description and required rights are updated. + * Saves a template with given name. If a template with that name already exists, + * the upload is rejected unless override=true is passed; in that case the existing + * template is updated in place (id preserved). * * @param file report file * @param name name of report * @param description report's description + * @param override when true, replace an existing template with the same name * @return saved report template */ public JasperTemplate saveTemplate( MultipartFile file, String name, String description, List requiredRights, - String category) throws ReportingException { + String category, Boolean override) throws ReportingException { validateRequiredRights(requiredRights); JasperTemplate jasperTemplate = jasperTemplateRepository.findByName(name); @@ -129,6 +132,10 @@ public JasperTemplate saveTemplate( .category(reportCategory.get()) .build(); } else { + if (!isTrue(override)) { + throw new ValidationMessageException( + new Message(ERROR_REPORTING_TEMPLATE_EXIST, name)); + } jasperTemplate.setDescription(description); jasperTemplate.getRequiredRights().clear(); jasperTemplate.getRequiredRights().addAll(requiredRights); @@ -400,15 +407,12 @@ void saveWithParameters(JasperTemplate jasperTemplate) { } /** - * Validate ".jrmxl" file and insert if template not exist. If this name of template already - * exist, remove older template and insert new. + * Validate ".jrxml" file and persist the template. Performs UPDATE in place + * when given a managed entity (preserves id). The previous delete-then-insert + * pattern caused a managed-entity conflict on duplicate-name uploads. */ void validateFileAndSaveTemplate(JasperTemplate jasperTemplate, MultipartFile file) throws ReportingException { - JasperTemplate templateTmp = jasperTemplateRepository.findByName(jasperTemplate.getName()); - if (templateTmp != null) { - jasperTemplateRepository.deleteById(templateTmp.getId()); - } validateFileAndSetData(jasperTemplate, file); saveWithParameters(jasperTemplate); } @@ -454,12 +458,14 @@ private void processJrParameters(JasperTemplate jasperTemplate, JRParameter[] jr throws ReportingException { ArrayList parameters = new ArrayList<>(); Set images = new HashSet<>(); + int order = 0; for (JRParameter jrParameter : jrParameters) { if (!jrParameter.isSystemDefined()) { if (jrParameter.isForPrompting()) { JasperTemplateParameter jasperTemplateParameter = createParameter(jrParameter); jasperTemplateParameter.setTemplate(jasperTemplate); + jasperTemplateParameter.setDisplayOrder(order++); parameters.add(jasperTemplateParameter); } else if (Image.class.getName().equals(jrParameter.getValueClassName())) { String name = jrParameter.getName(); @@ -472,8 +478,22 @@ private void processJrParameters(JasperTemplate jasperTemplate, JRParameter[] jr } } - jasperTemplate.setTemplateParameters(parameters); - jasperTemplate.setReportImages(images); + // Mutate existing collections instead of replacing references, otherwise + // Hibernate's orphanRemoval=true throws "collection no longer referenced" + // when called on a managed entity (override=true update path). + if (jasperTemplate.getTemplateParameters() == null) { + jasperTemplate.setTemplateParameters(new ArrayList<>()); + } else { + jasperTemplate.getTemplateParameters().clear(); + } + jasperTemplate.getTemplateParameters().addAll(parameters); + + if (jasperTemplate.getReportImages() == null) { + jasperTemplate.setReportImages(new HashSet<>()); + } else { + jasperTemplate.getReportImages().clear(); + } + jasperTemplate.getReportImages().addAll(images); } /** diff --git a/src/main/java/org/openlmis/report/web/JasperTemplateController.java b/src/main/java/org/openlmis/report/web/JasperTemplateController.java index 5bcc4c9..49451d8 100644 --- a/src/main/java/org/openlmis/report/web/JasperTemplateController.java +++ b/src/main/java/org/openlmis/report/web/JasperTemplateController.java @@ -111,7 +111,9 @@ public class JasperTemplateController extends BaseController { @ResponseStatus(HttpStatus.OK) public void createJasperReportTemplate( @RequestPart("file") MultipartFile file, String name, String description, - String[] requiredRights, String category) throws ReportingException { + String[] requiredRights, String category, + @RequestParam(value = "override", required = false) Boolean override) + throws ReportingException { permissionService.canEditReportTemplates(); LOGGER.debug("Saving template with name: " + name); @@ -120,7 +122,7 @@ public void createJasperReportTemplate( ? Collections.emptyList() : Arrays.asList(requiredRights); JasperTemplate template = jasperTemplateService - .saveTemplate(file, name, description, rightList, category); + .saveTemplate(file, name, description, rightList, category, override); LOGGER.debug("Saved template with id: " + template.getId()); } diff --git a/src/main/resources/api-definition.yaml b/src/main/resources/api-definition.yaml index 95c4e2a..d1e4811 100644 --- a/src/main/resources/api-definition.yaml +++ b/src/main/resources/api-definition.yaml @@ -282,6 +282,11 @@ resourceTypes: type: string required: false repeat: false + override: + displayName: Replace existing template with the same name + type: boolean + required: false + repeat: false responses: 200: 403: diff --git a/src/main/resources/db/migration/20260521102721443__add_display_order_to_template_parameters.sql b/src/main/resources/db/migration/20260521102721443__add_display_order_to_template_parameters.sql new file mode 100644 index 0000000..7c2be43 --- /dev/null +++ b/src/main/resources/db/migration/20260521102721443__add_display_order_to_template_parameters.sql @@ -0,0 +1 @@ +ALTER TABLE template_parameters ADD COLUMN displayorder INTEGER; diff --git a/src/main/resources/messages_en.properties b/src/main/resources/messages_en.properties index c1501b2..f5fb0e0 100644 --- a/src/main/resources/messages_en.properties +++ b/src/main/resources/messages_en.properties @@ -7,6 +7,8 @@ report.error.jasper.template.notFound=Jasper template not found in the system. report.error.jasper.report.generation=Error while generating Jasper report. report.error.jasper.report.format.unknown=Unknown report file format [{0}] specified. +report.error.reporting.template.exists=Jasper template with name "{0}" already exists. Pass override=true to replace it. + report.error.dashboardReport.name.duplicated=Dashboard report with name {0} already exists. report.error.dashboardReport.notFound=Dashboard report with id {0} can not be found. report.error.dashboardReport.id.mismatch=Error dashboard report id mismatch. diff --git a/src/test/java/org/openlmis/report/service/JasperTemplateServiceTest.java b/src/test/java/org/openlmis/report/service/JasperTemplateServiceTest.java index 8fbc920..e6c5770 100644 --- a/src/test/java/org/openlmis/report/service/JasperTemplateServiceTest.java +++ b/src/test/java/org/openlmis/report/service/JasperTemplateServiceTest.java @@ -158,6 +158,10 @@ public class JasperTemplateServiceTest { private static final String HEADER_CONFIG_PROPERTIES = "/config/reports/header_config.properties"; private static final String GLOBAL_HEADER_LANDSCAPE = "GlobalHeaderLandscape"; private static final String GLOBAL_HEADER_PORTRAIT = "GlobalHeaderPortrait"; + private static final String DESC = "desc"; + private static final String USERS_MANAGE = "USERS_MANAGE"; + private static final String JAVA_LANG_STRING = "java.lang.String"; + private static final String DEFAULT_EXPR_TEXT = "text"; private HttpServletRequest request; private JasperTemplate template; @@ -213,11 +217,57 @@ public void shouldUpdateValidExistentTemplate() throws Exception { assertEquals(template.getId(), oldId); } + @Test + public void shouldThrowWhenTemplateExistsAndOverrideIsNull() throws Exception { + expectedException.expect(ValidationMessageException.class); + expectedException.expectMessage(ERROR_REPORTING_TEMPLATE_EXIST); + + ReportCategory reportCategory = new ReportCategory(); + reportCategory.setId(UUID.randomUUID()); + reportCategory.setName(CATEGORY_NAME); + + JasperTemplate existing = new JasperTemplate(); + existing.setName(DISPLAY_NAME); + existing.setId(UUID.randomUUID()); + existing.setRequiredRights(new ArrayList<>()); + existing.setCategory(reportCategory); + + given(jasperTemplateRepository.findByName(anyString())).willReturn(existing); + given(reportCategoryRepository.findByName(anyString())).willReturn(Optional.of(reportCategory)); + given(rightReferenceDataService.findRight(anyString())).willReturn(new RightDto()); + + jasperTemplateService.saveTemplate(mock(MultipartFile.class), DISPLAY_NAME, DESC, + Collections.singletonList(USERS_MANAGE), CATEGORY_NAME, null); + } + + @Test + public void shouldThrowWhenTemplateExistsAndOverrideIsFalse() throws Exception { + expectedException.expect(ValidationMessageException.class); + expectedException.expectMessage(ERROR_REPORTING_TEMPLATE_EXIST); + + ReportCategory reportCategory = new ReportCategory(); + reportCategory.setId(UUID.randomUUID()); + reportCategory.setName(CATEGORY_NAME); + + JasperTemplate existing = new JasperTemplate(); + existing.setName(DISPLAY_NAME); + existing.setId(UUID.randomUUID()); + existing.setRequiredRights(new ArrayList<>()); + existing.setCategory(reportCategory); + + given(jasperTemplateRepository.findByName(anyString())).willReturn(existing); + given(reportCategoryRepository.findByName(anyString())).willReturn(Optional.of(reportCategory)); + given(rightReferenceDataService.findRight(anyString())).willReturn(new RightDto()); + + jasperTemplateService.saveTemplate(mock(MultipartFile.class), DISPLAY_NAME, DESC, + Collections.singletonList(USERS_MANAGE), CATEGORY_NAME, false); + } + private JasperTemplate testSaveTemplate() throws ReportingException { JasperTemplateService service = spy(jasperTemplateService); MultipartFile file = mock(MultipartFile.class); String description = "description"; - List requiredRights = Collections.singletonList("USERS_MANAGE"); + List requiredRights = Collections.singletonList(USERS_MANAGE); given(rightReferenceDataService.findRight(requiredRights.get(0))) .willReturn(new RightDto()); @@ -228,7 +278,7 @@ private JasperTemplate testSaveTemplate() throws ReportingException { // when JasperTemplate resultTemplate = service.saveTemplate(file, - JasperTemplateServiceTest.DISPLAY_NAME, description, requiredRights, CATEGORY_NAME); + JasperTemplateServiceTest.DISPLAY_NAME, description, requiredRights, CATEGORY_NAME, true); // then assertEquals(JasperTemplateServiceTest.DISPLAY_NAME, resultTemplate.getName()); @@ -249,7 +299,7 @@ public void shouldThrowErrorIfReportRequiredRightDoesNotExist() throws Exception // when jasperTemplateService.saveTemplate(null, null, null, Collections.singletonList(rejectedRight), - null); + null, false); } @Test @@ -391,18 +441,18 @@ public void shouldValidateFileAndSetData() throws Exception { when(propertiesMap.getProperty("options")).thenReturn("option 1,opt\\,ion 2"); when(param1.getPropertiesMap()).thenReturn(propertiesMap); - when(param1.getValueClassName()).thenReturn("java.lang.String"); + when(param1.getValueClassName()).thenReturn(JAVA_LANG_STRING); when(param1.getName()).thenReturn(PARAM_NAME); when(param1.isForPrompting()).thenReturn(true); - when(param1.getDescription()).thenReturn("desc"); + when(param1.getDescription()).thenReturn(DESC); when(param1.getDefaultValueExpression()).thenReturn(jrExpression); - when(jrExpression.getText()).thenReturn("text"); + when(jrExpression.getText()).thenReturn(DEFAULT_EXPR_TEXT); when(param2.getPropertiesMap()).thenReturn(propertiesMap); when(param2.getValueClassName()).thenReturn("java.lang.Integer"); when(param2.getName()).thenReturn(PARAM_NAME); when(param2.isForPrompting()).thenReturn(true); - when(param2.getDescription()).thenReturn("desc"); + when(param2.getDescription()).thenReturn(DESC); when(param2.getDefaultValueExpression()).thenReturn(jrExpression); when(param3.getValueClassName()).thenReturn("java.awt.Image"); @@ -429,7 +479,7 @@ public void shouldValidateFileAndSetData() throws Exception { assertEquals("test type", jasperTemplate.getType()); assertThat(jasperTemplate.getTemplateParameters().get(0).getDisplayName(), is(PARAM_DISPLAY_NAME)); - assertThat(jasperTemplate.getTemplateParameters().get(0).getDescription(), is("desc")); + assertThat(jasperTemplate.getTemplateParameters().get(0).getDescription(), is(DESC)); assertThat(jasperTemplate.getTemplateParameters().get(0).getName(), is(PARAM_NAME)); assertThat(jasperTemplate.getTemplateParameters().get(0).getRequired(), is(true)); assertThat(jasperTemplate.getTemplateParameters().get(0).getOptions(), contains("option 1", @@ -460,10 +510,10 @@ public void shouldValidateFileAndSetDataIfDefaultValueExpressionIsNull() throws when(propertiesMap.getProperty(DISPLAY_NAME)).thenReturn(PARAM_DISPLAY_NAME); when(param1.getPropertiesMap()).thenReturn(propertiesMap); - when(param1.getValueClassName()).thenReturn("java.lang.String"); + when(param1.getValueClassName()).thenReturn(JAVA_LANG_STRING); when(param1.isForPrompting()).thenReturn(true); when(param1.getDefaultValueExpression()).thenReturn(jrExpression); - when(jrExpression.getText()).thenReturn("text"); + when(jrExpression.getText()).thenReturn(DEFAULT_EXPR_TEXT); when(param2.getPropertiesMap()).thenReturn(propertiesMap); when(param2.getValueClassName()).thenReturn("java.lang.Integer"); @@ -622,11 +672,11 @@ public void shouldExtractDependenciesFromParameter() throws Exception { + "field2:contains:value2"); when(param1.getPropertiesMap()).thenReturn(propertiesMap); - when(param1.getValueClassName()).thenReturn("java.lang.String"); + when(param1.getValueClassName()).thenReturn(JAVA_LANG_STRING); when(param1.getName()).thenReturn(PARAM_NAME); when(param1.isForPrompting()).thenReturn(true); when(param1.getDefaultValueExpression()).thenReturn(jrExpression); - when(jrExpression.getText()).thenReturn("text"); + when(jrExpression.getText()).thenReturn(DEFAULT_EXPR_TEXT); ByteArrayOutputStream byteOutputStream = mock(ByteArrayOutputStream.class); whenNew(ByteArrayOutputStream.class).withAnyArguments().thenReturn(byteOutputStream); @@ -736,13 +786,13 @@ public void shouldThrowErrorIfCategoryNotFound() throws Exception { .willReturn(Optional.empty()); MultipartFile file = mock(MultipartFile.class); - List requiredRights = Collections.singletonList("USERS_MANAGE"); + List requiredRights = Collections.singletonList(USERS_MANAGE); given(rightReferenceDataService.findRight(requiredRights.get(0))) .willReturn(new RightDto()); jasperTemplateService.saveTemplate(file, "TestName", "Description", - requiredRights, "NonExistentCategory"); + requiredRights, "NonExistentCategory", false); } @Test @@ -770,13 +820,13 @@ public void shouldUpdateTemplateDescriptionAndRights() throws Exception { JasperTemplateService service = spy(jasperTemplateService); MultipartFile file = mock(MultipartFile.class); String newDescription = "New Description"; - List newRights = Collections.singletonList("USERS_MANAGE"); + List newRights = Collections.singletonList(USERS_MANAGE); doNothing().when(service) .validateFileAndSaveTemplate(any(JasperTemplate.class), eq(file)); JasperTemplate resultTemplate = service.saveTemplate(file, - DISPLAY_NAME, newDescription, newRights, CATEGORY_NAME); + DISPLAY_NAME, newDescription, newRights, CATEGORY_NAME, true); assertEquals(newDescription, resultTemplate.getDescription()); assertEquals(newRights, resultTemplate.getRequiredRights()); @@ -1332,6 +1382,69 @@ public void getGlobalHeaderShouldReturnCompiledHeaderAndDynamicParams() throws E assertEquals("/config/reports/logo.png", result.get("logoImage")); } + @Test + public void shouldSetDisplayOrderFromJrxmlDeclarationOrder() throws Exception { + MultipartFile file = mock(MultipartFile.class); + when(file.getOriginalFilename()).thenReturn(NAME_OF_FILE); + + mockStatic(JasperCompileManager.class); + JasperReport report = mock(JasperReport.class); + InputStream inputStream = mock(InputStream.class); + when(file.getInputStream()).thenReturn(inputStream); + + JRParameter first = mock(JRParameter.class); + JRParameter imageParam = mock(JRParameter.class); + JRParameter second = mock(JRParameter.class); + JRParameter third = mock(JRParameter.class); + JRPropertiesMap propertiesMap = mock(JRPropertiesMap.class); + JRExpression jrExpression = mock(JRExpression.class); + + String[] propertyNames = {DISPLAY_NAME}; + when(report.getParameters()) + .thenReturn(new JRParameter[]{first, imageParam, second, third}); + when(JasperCompileManager.compileReport(inputStream)).thenReturn(report); + when(propertiesMap.getPropertyNames()).thenReturn(propertyNames); + when(propertiesMap.getProperty(DISPLAY_NAME)).thenReturn(PARAM_DISPLAY_NAME); + when(jrExpression.getText()).thenReturn(DEFAULT_EXPR_TEXT); + + for (JRParameter p : new JRParameter[]{first, second, third}) { + when(p.getPropertiesMap()).thenReturn(propertiesMap); + when(p.getValueClassName()).thenReturn(JAVA_LANG_STRING); + when(p.isForPrompting()).thenReturn(true); + when(p.getDefaultValueExpression()).thenReturn(jrExpression); + } + when(first.getName()).thenReturn(PARAM1); + when(second.getName()).thenReturn(PARAM2); + when(third.getName()).thenReturn(PARAM3); + + when(imageParam.getValueClassName()).thenReturn("java.awt.Image"); + when(imageParam.isForPrompting()).thenReturn(false); + when(imageParam.isSystemDefined()).thenReturn(false); + when(imageParam.getName()).thenReturn(IMAGE_NAME); + when(reportImageRepository.findByName(IMAGE_NAME)).thenReturn(mock(ReportImage.class)); + + ByteArrayOutputStream byteOutputStream = mock(ByteArrayOutputStream.class); + whenNew(ByteArrayOutputStream.class).withAnyArguments().thenReturn(byteOutputStream); + ObjectOutputStream objectOutputStream = spy(new ObjectOutputStream(byteOutputStream)); + whenNew(ObjectOutputStream.class).withArguments(byteOutputStream) + .thenReturn(objectOutputStream); + doNothing().when(objectOutputStream).writeObject(report); + when(byteOutputStream.toByteArray()).thenReturn(new byte[1]); + + JasperTemplate jasperTemplate = new JasperTemplate(); + + jasperTemplateService.validateFileAndInsertTemplate(jasperTemplate, file); + + List params = jasperTemplate.getTemplateParameters(); + assertEquals(3, params.size()); + assertEquals(PARAM1, params.get(0).getName()); + assertEquals(Integer.valueOf(0), params.get(0).getDisplayOrder()); + assertEquals(PARAM2, params.get(1).getName()); + assertEquals(Integer.valueOf(1), params.get(1).getDisplayOrder()); + assertEquals(PARAM3, params.get(2).getName()); + assertEquals(Integer.valueOf(2), params.get(2).getDisplayOrder()); + } + private byte[] convertImageToByteArray(BufferedImage image) throws IOException { ByteArrayOutputStream os = new ByteArrayOutputStream(); ImageIO.write(image, "png", os);