Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: limits for category combinations [DHIS2-18521] #19374

Merged
merged 5 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -51,7 +51,7 @@
public class CategoryCombo extends BaseIdentifiableObject implements SystemDefaultMetadataObject {
public static final String DEFAULT_CATEGORY_COMBO_NAME = "default";

/** A set with categories. */
/** The categories combined in this combo in the order they are used as a category combination */
private List<Category> categories = new ArrayList<>();

/**
Expand Down Expand Up @@ -243,7 +243,7 @@
@JsonSerialize(contentAs = BaseIdentifiableObject.class)
@JacksonXmlElementWrapper(localName = "categoryOptionCombos", namespace = DxfNamespaces.DXF_2_0)
@JacksonXmlProperty(localName = "categoryOptionCombo", namespace = DxfNamespaces.DXF_2_0)
public Set<CategoryOptionCombo> getOptionCombos() {

Check notice

Code scanning / CodeQL

Exposing internal representation Note

getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation stored in field optionCombos. The value may be modified
after this call to getOptionCombos
.
getOptionCombos exposes the internal representation sto
return optionCombos;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,22 @@
package org.hisp.dhis.category;

import java.util.List;
import javax.annotation.CheckForNull;
import org.hisp.dhis.common.IdentifiableObjectStore;
import org.hisp.dhis.common.UID;
import org.hisp.dhis.user.UserDetails;

/**
* @author Lars Helge Overland
*/
public interface CategoryOptionStore extends IdentifiableObjectStore<CategoryOption> {

/**
* @param category the UID of the category
* @return the number of category options in a category
*/
int getCategoryOptionsCount(@CheckForNull UID category);

List<CategoryOption> getCategoryOptions(Category category);

List<CategoryOption> getDataWriteCategoryOptions(Category category, UserDetails userDetails);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.hisp.dhis.dataelement.DataElement;
import org.hisp.dhis.dataelement.DataElementOperand;
import org.hisp.dhis.dataset.DataSet;
import org.hisp.dhis.feedback.ConflictException;
import org.hisp.dhis.user.User;
import org.hisp.dhis.user.UserDetails;

Expand Down Expand Up @@ -563,4 +564,10 @@ List<CategoryOptionCombo> getCategoryOptionCombosByCategoryOption(
Category getDefaultCategory();

List<CategoryOption> getCategoryOptionsByUid(List<String> catOptionUids);

void validate(Category category) throws ConflictException;

void validate(CategoryCombo combo) throws ConflictException;

void validate(CategoryOptionCombo combo) throws ConflictException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.hisp.dhis.common.OpenApi.Response.Status.CONFLICT;

import java.text.MessageFormat;
import java.util.List;
import java.util.function.Function;
import java.util.function.Supplier;
import lombok.Getter;
Expand All @@ -55,6 +56,7 @@ public static <E extends RuntimeException, V> V on(
}

private final ErrorCode code;
private final Object[] args;

@Setter private String devMessage;

Expand All @@ -70,15 +72,19 @@ public ConflictException(String message, String devMessage) {
super(message);
this.devMessage = devMessage;
this.code = ErrorCode.E1004;
this.args = new Object[0];
}

public ConflictException(ErrorCode code, Object... args) {
super(MessageFormat.format(code.getMessage(), args));
this.code = code;
this.args = args;
}

public ConflictException(ErrorMessage message) {
super(message.getMessage());
this.code = message.getErrorCode();
List<String> args = message.getArgs();
this.args = args == null ? new Object[0] : args.toArray();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ public enum ErrorCode {

E1122("Category option combo {0} cannot be associated with the default category combo"),
E1125("Category option combo {0} contains options not associated with category combo {1}"),
E1126("Category combo {0} cannot combine more than {1} categories, but had: {2}"),
E1127("Category {0} cannot have more than {1} options, but had: {2} "),
E1128("Category combo {0} cannot have more than {1} combinations, bud requires: {2}"),
Copy link
Contributor

@david-mackessy david-mackessy Dec 5, 2024

Choose a reason for hiding this comment

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

maybe this message can mirror the other 2?
..... cannot have more than x combinations, but had: y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is worded differently because it is different. The combinations do not exist (yet) and when the validation fails we also don't create them, so I thought wording it "but had" is confusing if you know what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool. There is a typo in bud. Should be but.


/* Org unit merge */
E1500("At least two source orgs unit must be specified"),
E1501("Target org unit must be specified"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import static org.hisp.dhis.system.deletion.DeletionVeto.ACCEPT;

import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.system.deletion.DeletionVeto;
import org.hisp.dhis.system.deletion.IdObjectDeletionHandler;
Expand Down Expand Up @@ -58,8 +59,10 @@ private DeletionVeto allowDeleteCategory(Category category) {
}

private void deleteCategoryOptionCombo(CategoryOptionCombo categoryOptionCombo) {
for (CategoryCombo categoryCombo : categoryService.getAllCategoryCombos()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I am not sure why we would loop over all CCs when there is just one linked to the COC which needs unlinking. Also I added a check if the un-linking isn't already done because if this was called as a consequence of deleting the CC it has and so the update should not be called again on the CC.

Copy link
Contributor

Choose a reason for hiding this comment

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

If not needed, this is very nice. That could be very costly.
An existing/new test would confirm that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I only have my subjective observation from test and debugging but I found this because an deletion attempt would just spin my PC out of control so I had to kill the server, then step through it to see what was going on, which was when I found this. I then redid the deletion and it still was super slow but the server was at least responsive.

categoryCombo.getOptionCombos().remove(categoryOptionCombo);
CategoryCombo categoryCombo = categoryOptionCombo.getCategoryCombo();
Set<CategoryOptionCombo> optionCombos = categoryCombo.getOptionCombos();
if (optionCombos.contains(categoryOptionCombo)) {
optionCombos.remove(categoryOptionCombo);
idObjectManager.updateNoAcl(categoryCombo);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
import org.hisp.dhis.dataelement.DataElementOperand;
import org.hisp.dhis.dataset.DataSet;
import org.hisp.dhis.dataset.DataSetElement;
import org.hisp.dhis.external.conf.ConfigurationKey;
import org.hisp.dhis.external.conf.DhisConfigurationProvider;
import org.hisp.dhis.feedback.ConflictException;
import org.hisp.dhis.feedback.ErrorCode;
import org.hisp.dhis.security.acl.AccessStringHelper;
import org.hisp.dhis.security.acl.AclService;
import org.hisp.dhis.user.CurrentUserUtil;
Expand All @@ -64,25 +68,16 @@
@Service("org.hisp.dhis.category.CategoryService")
@RequiredArgsConstructor
public class DefaultCategoryService implements CategoryService {
// -------------------------------------------------------------------------
// Dependencies
// -------------------------------------------------------------------------

private final CategoryStore categoryStore;

private final CategoryOptionStore categoryOptionStore;

private final CategoryComboStore categoryComboStore;

private final CategoryOptionComboStore categoryOptionComboStore;

private final CategoryOptionGroupStore categoryOptionGroupStore;

private final CategoryOptionGroupSetStore categoryOptionGroupSetStore;

private final IdentifiableObjectManager idObjectManager;

private final AclService aclService;
private final DhisConfigurationProvider configuration;

@Qualifier("jdbcCategoryOptionOrgUnitAssociationsStore")
private final JdbcOrgUnitAssociationsStore jdbcOrgUnitAssociationsStore;
Expand All @@ -91,6 +86,17 @@ public class DefaultCategoryService implements CategoryService {
// Category
// -------------------------------------------------------------------------

@Override
@Transactional(readOnly = true)
public void validate(Category category) throws ConflictException {
int maxOptions = configuration.getIntProperty(ConfigurationKey.METADATA_CATEGORIES_MAX_OPTIONS);
int actualOptions = category.getCategoryOptions().size();
if (actualOptions == 0) // assume a transient object that does not have options set
actualOptions = categoryOptionStore.getCategoryOptionsCount(UID.of(category.getUid()));
if (actualOptions > maxOptions)
throw new ConflictException(ErrorCode.E1127, category.getUid(), maxOptions, actualOptions);
}

@Override
@Transactional
public long addCategory(Category dataElementCategory) {
Expand Down Expand Up @@ -314,6 +320,29 @@ public List<CategoryOption> getCategoryOptionsByUid(List<String> catOptionUids)
// CategoryCombo
// -------------------------------------------------------------------------

@Override
@Transactional(readOnly = true)
public void validate(CategoryCombo combo) throws ConflictException {
int maxCategories =
configuration.getIntProperty(ConfigurationKey.METADATA_CATEGORIES_MAX_PER_COMBO);
int actualCategories = combo.getCategories().size();
if (actualCategories > maxCategories)
throw new ConflictException(ErrorCode.E1126, combo.getUid(), maxCategories, actualCategories);
for (Category c : combo.getCategories()) validate(c);
int maxCombinations =
configuration.getIntProperty(ConfigurationKey.METADATA_CATEGORIES_MAX_COMBINATIONS);
int actualCombinations = 1;
for (Category c : combo.getCategories()) {
int options = c.getCategoryOptions().size();
if (options == 0) // assume c is a transient object that has no options set
options = categoryOptionStore.getCategoryOptionsCount(UID.of(c.getUid()));
actualCombinations *= options;
}
if (actualCombinations > maxCombinations)
throw new ConflictException(
ErrorCode.E1128, combo.getUid(), maxCombinations, actualCombinations);
}

@Override
@Transactional
public long addCategoryCombo(CategoryCombo dataElementCategoryCombo) {
Expand Down Expand Up @@ -426,6 +455,12 @@ public String validateCategoryCombo(CategoryCombo categoryCombo) {
// CategoryOptionCombo
// -------------------------------------------------------------------------

@Override
@Transactional(readOnly = true)
public void validate(CategoryOptionCombo combo) throws ConflictException {
validate(combo.getCategoryCombo());
}

@Override
@Transactional
public long addCategoryOptionCombo(CategoryOptionCombo dataElementCategoryOptionCombo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.hisp.dhis.category.Category;
import org.hisp.dhis.category.CategoryOption;
import org.hisp.dhis.category.CategoryOptionStore;
import org.hisp.dhis.common.UID;
import org.hisp.dhis.common.hibernate.HibernateIdentifiableObjectStore;
import org.hisp.dhis.security.acl.AclService;
import org.hisp.dhis.user.UserDetails;
Expand All @@ -54,6 +55,19 @@ public HibernateCategoryOptionStore(
super(entityManager, jdbcTemplate, publisher, CategoryOption.class, aclService, true);
}

@Override
public int getCategoryOptionsCount(UID category) {
if (category == null) return 0;
String sql =
"""
select count(*) from categories_categoryoptions co
left join category c on c.categoryid = co.categoryid
where c.uid = :id""";
Object count =
nativeSynchronizedQuery(sql).setParameter("id", category.getValue()).getSingleResult();
return count instanceof Number n ? n.intValue() : 0;
}

@Override
public List<CategoryOption> getCategoryOptions(Category category) {
CriteriaBuilder builder = getCriteriaBuilder();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2004-2024, University of Oslo
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
* Neither the name of the HISP project nor the names of its contributors may
* be used to endorse or promote products derived from this software without
* specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package org.hisp.dhis.dxf2.metadata.objectbundle.hooks;

import java.util.function.Consumer;
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.category.CategoryCombo;
import org.hisp.dhis.category.CategoryOptionCombo;
import org.hisp.dhis.category.CategoryService;
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
import org.hisp.dhis.feedback.ConflictException;
import org.hisp.dhis.feedback.ErrorReport;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
public class CategoryComboObjectBundleHook extends AbstractObjectBundleHook<CategoryCombo> {

private final CategoryService categoryService;

@Override
public void validate(CategoryCombo combo, ObjectBundle bundle, Consumer<ErrorReport> addReports) {
checkIsValid(combo, addReports);
}

private void checkIsValid(CategoryCombo combo, Consumer<ErrorReport> addReports) {
try {
categoryService.validate(combo);
} catch (ConflictException ex) {
addReports.accept(new ErrorReport(CategoryOptionCombo.class, ex.getCode(), ex.getArgs()));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2004-2024, University of Oslo
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
* Neither the name of the HISP project nor the names of its contributors may
* be used to endorse or promote products derived from this software without
* specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package org.hisp.dhis.dxf2.metadata.objectbundle.hooks;

import java.util.function.Consumer;
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.category.Category;
import org.hisp.dhis.category.CategoryOptionCombo;
import org.hisp.dhis.category.CategoryService;
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
import org.hisp.dhis.feedback.ConflictException;
import org.hisp.dhis.feedback.ErrorReport;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
public class CategoryObjectBundleHook extends AbstractObjectBundleHook<Category> {

private final CategoryService categoryService;

@Override
public void validate(Category category, ObjectBundle bundle, Consumer<ErrorReport> addReports) {
checkIsValid(category, addReports);
}

private void checkIsValid(Category category, Consumer<ErrorReport> addReports) {
try {
categoryService.validate(category);
} catch (ConflictException ex) {
addReports.accept(new ErrorReport(CategoryOptionCombo.class, ex.getCode(), ex.getArgs()));
}
}
}
Loading
Loading