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.

Sign up for GitHub

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

fix: program rule variable name check now also check UID to prevent duplicates [DHIS2-11330] #8220

Merged
merged 6 commits into from
Jun 22, 2021
Merged

fix: program rule variable name check now also check UID to prevent duplicates [DHIS2-11330] #8220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94a06d4
fix: program rule variable name check should be now properly implemented
gnespolino Jun 14, 2021
a37363d
fix: changed check implementation to tolerate bad database data
gnespolino Jun 14, 2021
7d76b02
fix: added test case
gnespolino Jun 14, 2021
c4f9f6b
style: formatting
gnespolino Jun 14, 2021
60715d4
fix: compilation issue with JDK 9
gnespolino Jun 21, 2021
fd11e98
style: formatting
gnespolino Jun 21, 2021
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
44 changes: 38 additions & 6 deletions ...a/org/hisp/dhis/dxf2/metadata/objectbundle/hooks/ProgramRuleVariableObjectBundleHook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@

import static org.hisp.dhis.dxf2.Constants.PROGRAM_RULE_VARIABLE_NAME_INVALID_KEYWORDS;

import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.IntStream;

import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
import org.hibernate.Session;
import org.hibernate.query.Query;
import org.hisp.dhis.common.IdentifiableObject;
Expand Down Expand Up @@ -94,18 +97,47 @@ public <T extends IdentifiableObject> void validate( T object, ObjectBundle bund
private void validateUniqueProgramRuleName( ObjectBundle bundle,
ProgramRuleVariable programRuleVariable, Consumer<ErrorReport> addReports )
{
Query<ProgramRuleVariable> query = getProgramRuleVariableQuery( programRuleVariable );
List<ProgramRuleVariable> prvWithSameNameAndSameProgram = getProgramRuleVariableQuery( programRuleVariable )
.getResultList();
Comment on lines 99 to +101
Copy link
Contributor

@lucaCambi77 lucaCambi77 Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this approach is too verbose, in my opinion we should simply try to do a query also using prv UID.
After that if it is a create strategy and we get a record, we have an error. We also have an error if we don't get a result and it is an update.
I think this way is much cleaner, less code and maintenable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, I'll review it

Copy link
Contributor Author

@gnespolino gnespolino Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

better thinking, we should

  • fail when it's an insert and a PRV with same name/program already exists
  • fail when it's an update and a PRV with same name/program/uid already exists

so I think it's not possible to verify the first check adding UID in the query filter


int allowedCount = UPDATE_STRATEGIES.contains( bundle.getImportMode() ) ? 1 : 0;
// update
if ( UPDATE_STRATEGIES.contains( bundle.getImportMode() ) )
{
if ( !isLegitUpdate( programRuleVariable, prvWithSameNameAndSameProgram ) )
{
failPrvWithSameNAmeAlreadyExists( programRuleVariable, addReports );
}
return;
}

if ( query.getResultList().size() > allowedCount )
// insert
if ( CollectionUtils.isNotEmpty( prvWithSameNameAndSameProgram ) )
{
addReports.accept(
new ErrorReport( ProgramRuleVariable.class, ErrorCode.E4051, programRuleVariable.getName(),
programRuleVariable.getProgram().getUid() ) );
failPrvWithSameNAmeAlreadyExists( programRuleVariable, addReports );
}
}

private boolean isLegitUpdate( ProgramRuleVariable programRuleVariable,
List<ProgramRuleVariable> existingPrvs )
{
return existingPrvs.isEmpty() ||
existingPrvs.stream()
.anyMatch( existingPrv -> hasSameUid( existingPrv, programRuleVariable ) );
}

private boolean hasSameUid( ProgramRuleVariable existingPrv, ProgramRuleVariable programRuleVariable )
{
return StringUtils.equals( existingPrv.getUid(), programRuleVariable.getUid() );
}

private void failPrvWithSameNAmeAlreadyExists( ProgramRuleVariable programRuleVariable,
Consumer<ErrorReport> addReports )
{
addReports.accept(
new ErrorReport( ProgramRuleVariable.class, ErrorCode.E4051, programRuleVariable.getName(),
programRuleVariable.getProgram().getUid() ) );
}

private Query<ProgramRuleVariable> getProgramRuleVariableQuery( ProgramRuleVariable programRuleVariable )
{
Session session = sessionFactory.getCurrentSession();
Expand Down
64 changes: 61 additions & 3 deletions ...g/hisp/dhis/dxf2/metadata/objectbundle/hooks/ProgramRuleVariableObjectBundleHookTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import com.google.common.collect.ImmutableList;

/**
* @author Luca Cambi
*/
Expand Down Expand Up @@ -108,7 +110,7 @@ public void shouldExitObjectNotInstanceOfProgramRuleVariable()
}

@Test
public void shouldFailValidationInvalidCount()
public void shouldFailInsertAlreadyExisting()
{
when( programRuleVariable.getProgram() ).thenReturn( program );
when( objectBundle.getImportMode() ).thenReturn( ImportStrategy.CREATE );
Expand All @@ -122,18 +124,74 @@ public void shouldFailValidationInvalidCount()
}

@Test
public void shouldNotFailValidationInvalidCount()
public void shouldNotFailUpdateExistingSameUid()
{
when( programRuleVariable.getProgram() ).thenReturn( program );
when( objectBundle.getImportMode() ).thenReturn( ImportStrategy.CREATE_AND_UPDATE );
when( query.getResultList() ).thenReturn( Collections.singletonList( new ProgramRuleVariable() ) );

ProgramRuleVariable existingProgramRuleVariable = new ProgramRuleVariable();
existingProgramRuleVariable.setName( "word" );
existingProgramRuleVariable.setUid( "uid1" );

when( query.getResultList() ).thenReturn( Collections.singletonList( existingProgramRuleVariable ) );

when( programRuleVariable.getName() ).thenReturn( "word" );
when( programRuleVariable.getUid() ).thenReturn( "uid1" );

List<ErrorReport> errorReports = programRuleVariableObjectBundleHook.validate( programRuleVariable,
objectBundle );

assertEquals( 0, errorReports.size() );
}

@Test
public void shouldNotFailUpdateExistingMoreThanOneSameUid()
{
when( programRuleVariable.getProgram() ).thenReturn( program );
when( objectBundle.getImportMode() ).thenReturn( ImportStrategy.CREATE_AND_UPDATE );

ProgramRuleVariable existingProgramRuleVariable = new ProgramRuleVariable();
existingProgramRuleVariable.setName( "word" );
existingProgramRuleVariable.setUid( "uid1" );

ProgramRuleVariable anotherExistingProgramRuleVariable = new ProgramRuleVariable();
anotherExistingProgramRuleVariable.setName( "word" );
anotherExistingProgramRuleVariable.setUid( "uid2" );

when( query.getResultList() )
.thenReturn( ImmutableList.of( existingProgramRuleVariable, anotherExistingProgramRuleVariable ) );

when( programRuleVariable.getName() ).thenReturn( "word" );
when( programRuleVariable.getUid() ).thenReturn( "uid1" );

List<ErrorReport> errorReports = programRuleVariableObjectBundleHook.validate( programRuleVariable,
objectBundle );

assertEquals( 0, errorReports.size() );
}

@Test
public void shouldFailUpdateExistingDifferentUid()
{
when( programRuleVariable.getProgram() ).thenReturn( program );
when( objectBundle.getImportMode() ).thenReturn( ImportStrategy.CREATE_AND_UPDATE );

ProgramRuleVariable existingProgramRuleVariable = new ProgramRuleVariable();
existingProgramRuleVariable.setName( "word" );
existingProgramRuleVariable.setUid( "uid1" );

when( query.getResultList() ).thenReturn( Collections.singletonList( existingProgramRuleVariable ) );

when( programRuleVariable.getName() ).thenReturn( "word" );
when( programRuleVariable.getUid() ).thenReturn( "uid2" );

List<ErrorReport> errorReports = programRuleVariableObjectBundleHook.validate( programRuleVariable,
objectBundle );

assertEquals( 1, errorReports.size() );
assertTrue( errorReports.stream().anyMatch( e -> e.getErrorCode().equals( E4051 ) ) );
}

@Test
public void shouldFailValidationInvalidCountAndInvalidName()
{
Expand Down