Repository: adessose/budgeteer
Issue: Risk of calling WorkRecordRepository.updateDailyRates with null values

When updating the daily rates in the PersonService by calling the updateDailyRates method in the WorkRecordRepository there is the risk of executing this query with null values.

This code snippet show the issue:
`    workRecordRepository.updateDailyRates(rate.getBudget().getId(), person.getPersonId(),
            rate.getDateRange().getStartDate(), rate.getDateRange().getEndDate(), rate.getRate());`

The rate is an instance of the class PersonRate, which got a NoArgsConstructor and a reset method. That's why there is the possibility of null values.
Additionally, it should be investigated if the reset method should remain. By resetting a PersonRate there is the chance of inconsistent data because resetting the rate loses the connection to the WorkRecords. Then the rate remains in the work records but the rate itself is reset.

Changed files:
budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonRate.java
budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonService.java
budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/person/edit/personrateform/EditPersonForm.java
budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/person/PersonServiceIntegrationTest.java

diff --git a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonRate.java b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonRate.java
index 21289f46..951a6929 100644
--- a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonRate.java
+++ b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonRate.java
@@ -1,11 +1,10 @@
 package org.wickedsource.budgeteer.service.person;
 
+import lombok.AccessLevel;
 import lombok.AllArgsConstructor;
 import lombok.Data;
 import lombok.NoArgsConstructor;
 import lombok.experimental.Accessors;
-import org.apache.commons.lang3.builder.EqualsBuilder;
-import org.apache.commons.lang3.builder.HashCodeBuilder;
 import org.joda.money.Money;
 import org.wickedsource.budgeteer.service.DateRange;
 import org.wickedsource.budgeteer.service.budget.BudgetBaseData;
@@ -13,30 +12,11 @@ import org.wickedsource.budgeteer.service.budget.BudgetBaseData;
 import java.io.Serializable;
 
 @Data
-@NoArgsConstructor
+@NoArgsConstructor(access = AccessLevel.NONE)
 @AllArgsConstructor
 @Accessors(chain = true)
 public class PersonRate implements Serializable {
-
     private Money rate;
     private BudgetBaseData budget;
     private DateRange dateRange;
-
-    @Override
-    @SuppressWarnings("EqualsWhichDoesntCheckParameterClass")
-    public boolean equals(Object obj) {
-        return EqualsBuilder.reflectionEquals(this, obj);
-    }
-
-    @Override
-    public int hashCode() {
-        return HashCodeBuilder.reflectionHashCode(this);
-    }
-
-    public void reset(){
-        rate = null;
-        budget = null;
-        dateRange = null;
-    }
-
 }
diff --git a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonService.java b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonService.java
index 05b267a7..918aa6c0 100644
--- a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonService.java
+++ b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/service/person/PersonService.java
@@ -20,7 +20,9 @@ import org.wickedsource.budgeteer.web.planning.Person;
 
 import javax.transaction.Transactional;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 
 
 @Service
@@ -85,10 +87,9 @@ public class PersonService {
             budget.setId(rateEntity.getBudget().getId());
             budget.setName(rateEntity.getBudget().getName());
 
-            PersonRate rate = new PersonRate();
-            rate.setBudget(budget);
-            rate.setDateRange(new DateRange(rateEntity.getDateStart(), rateEntity.getDateEnd()));
-            rate.setRate(rateEntity.getRate());
+            PersonRate rate = new PersonRate(rateEntity.getRate(),
+                    budget,
+                    new DateRange(rateEntity.getDateStart(), rateEntity.getDateEnd()));
 
             person.getRates().add(rate);
         }
@@ -101,15 +102,24 @@ public class PersonService {
      *
      * @param person the data to save in the database.
      */
-    public void savePersonWithRates(PersonWithRates person) {
+    public List<String> savePersonWithRates(PersonWithRates person) {
         PersonEntity personEntity = personRepository.findOne(person.getPersonId());
         personEntity.setName(person.getName());
         personEntity.setImportKey(person.getImportKey());
         personEntity.setDefaultDailyRate(person.getDefaultDailyRate());
 
         List<DailyRateEntity> dailyRates = new ArrayList<>();
-
+        List<String> errors = new ArrayList<>();
         for (PersonRate rate : person.getRates()) {
+            List<String> errorList = this.updateDailyRates(rate.getBudget(),
+                    person,
+                    rate.getDateRange(),
+                    rate.getRate());
+
+            if (!errorList.isEmpty()) {
+                errors.addAll(errorList);
+                continue;
+            }
 
             DailyRateEntity rateEntity = new DailyRateEntity();
             rateEntity.setRate(rate.getRate());
@@ -118,14 +128,12 @@ public class PersonService {
             rateEntity.setDateStart(rate.getDateRange().getStartDate());
             rateEntity.setDateEnd(rate.getDateRange().getEndDate());
             dailyRates.add(rateEntity);
-            workRecordRepository.updateDailyRates(rate.getBudget().getId(), person.getPersonId(),
-                    rate.getDateRange().getStartDate(), rate.getDateRange().getEndDate(), rate.getRate());
-
 
         }
         personEntity.getDailyRates().clear();
         personEntity.getDailyRates().addAll(dailyRates);
         personRepository.save(personEntity);
+        return errors;
     }
 
     public List<String> getOverlapWithManuallyEditedRecords(PersonWithRates person, long projectId){
@@ -178,7 +186,33 @@ public class PersonService {
     }
 
     public void removeDailyRateFromPerson(PersonWithRates personWithRates, PersonRate rate) {
-        workRecordRepository.updateDailyRates(rate.getBudget().getId(), personWithRates.getPersonId(),
-                rate.getDateRange().getStartDate(), rate.getDateRange().getEndDate(), Money.zero(CurrencyUnit.EUR));
+        this.updateDailyRates(rate.getBudget(), personWithRates, rate.getDateRange(), Money.zero(CurrencyUnit.EUR));
+    }
+
+    private List<String> updateDailyRates(BudgetBaseData budget, PersonWithRates person, DateRange dateRange, Money dailyRate) {
+        List<String> errorMessages = new ArrayList<>();
+        if (budget == null) {
+            errorMessages.add("Budget shouldn't be null!");
+        }
+        if (person == null){
+            errorMessages.add("Person shouldn't be null!");
+        }
+        if (dailyRate == null) {
+            errorMessages.add("Daily Rate shouldn't be null!");
+        }
+        if (dateRange == null || dateRange.getStartDate() == null || dateRange.getEndDate() == null) {
+            errorMessages.add("Date Range shouldn't be null!");
+        }
+
+        if (!errorMessages.isEmpty()) {
+            errorMessages.add("The Daily Rate wasn't updated because of invalid values");
+            return errorMessages;
+        }
+        workRecordRepository.updateDailyRates(budget.getId(),
+                person.getPersonId(),
+                dateRange.getStartDate(),
+                dateRange.getEndDate(),
+                dailyRate);
+        return Collections.emptyList();
     }
 }
diff --git a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/person/edit/personrateform/EditPersonForm.java b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/person/edit/personrateform/EditPersonForm.java
index 569f2464..ca77b2ac 100644
--- a/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/person/edit/personrateform/EditPersonForm.java
+++ b/budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/person/edit/personrateform/EditPersonForm.java
@@ -150,12 +150,15 @@ public class EditPersonForm extends Form<PersonWithRates> {
                     EditPersonForm.this.getModelObject().setName(nameTextField.getInput());
                     EditPersonForm.this.getModelObject().setImportKey(importKeyTextField.getInput());
                     EditPersonForm.this.getModelObject().setDefaultDailyRate(defaultDailyRate);
-                    personService.savePersonWithRates(EditPersonForm.this.getModelObject());
+                    List<String> errors = personService.savePersonWithRates(EditPersonForm.this.getModelObject());
                     List<String> warnings = personService.getOverlapWithManuallyEditedRecords(EditPersonForm.this.getModelObject(),
                             BudgeteerSession.get().getProjectId());
                     this.success(getString("form.success"));
-                    for (String e : warnings) {
-                        this.info(e);
+                    for (String warning : warnings) {
+                        this.info(warning);
+                    }
+                    for (String error : errors) {
+                        this.error(error);
                     }
                 }
                 showMissingDailyRateContainer();
@@ -193,10 +196,10 @@ public class EditPersonForm extends Form<PersonWithRates> {
                 }
 
                 List<PersonRate> missingRates = missingDailyRateForBudgetBeans.stream()
-                        .map(missingDailyRate -> new PersonRate()
-                                .setBudget(budgetService.loadBudgetBaseData(missingDailyRate.getBudgetId()))
-                                .setRate(currentDefaultDailyRate)
-                                .setDateRange(new DateRange(missingDailyRate.getStartDate(), missingDailyRate.getEndDate())))
+                        .map(missingDailyRate -> new PersonRate(currentDefaultDailyRate,
+                                budgetService.loadBudgetBaseData(missingDailyRate.getBudgetId()),
+                                new DateRange(missingDailyRate.getStartDate(), missingDailyRate.getEndDate())
+                        ))
                         .collect(Collectors.toList());
 
                 EditPersonForm.this.getModelObject().getRates().addAll(missingRates);
diff --git a/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/person/PersonServiceIntegrationTest.java b/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/person/PersonServiceIntegrationTest.java
index 6bee7e12..26cbc79c 100644
--- a/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/person/PersonServiceIntegrationTest.java
+++ b/budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/service/person/PersonServiceIntegrationTest.java
@@ -56,15 +56,13 @@ class PersonServiceIntegrationTest extends IntegrationTestTemplate {
         person.setPersonId(1L);
         person.setName("name");
         person.setImportKey("name");
-        PersonRate rate1 = new PersonRate();
-        rate1.setRate(MoneyUtil.createMoneyFromCents(12300L));
-        rate1.setDateRange(new DateRange(format.parse("01.01.2014"), format.parse("15.08.2014")));
-        rate1.setBudget(new BudgetBaseData(1L, "budget1"));
+        PersonRate rate1 = new PersonRate(MoneyUtil.createMoneyFromCents(12300L),
+                new BudgetBaseData(1L, "budget1"),
+                new DateRange(format.parse("01.01.2014"), format.parse("15.08.2014")));
         person.getRates().add(rate1);
-        PersonRate rate2 = new PersonRate();
-        rate2.setRate(MoneyUtil.createMoneyFromCents(32100L));
-        rate2.setDateRange(new DateRange(format.parse("01.01.2015"), format.parse("15.08.2015")));
-        rate2.setBudget(new BudgetBaseData(1L, "budget1"));
+        PersonRate rate2 = new PersonRate(MoneyUtil.createMoneyFromCents(32100L),
+                new BudgetBaseData(1L, "budget1"),
+                new DateRange(format.parse("01.01.2015"), format.parse("15.08.2015")));
         person.getRates().add(rate2);
         service.savePersonWithRates(person);