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

Feature/kob 66 angebote stadtweit #19

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
5a3614e
refactored
ThomasAFink May 12, 2023
543bb7c
refactored
ThomasAFink May 12, 2023
c4f14b3
changed tests
ThomasAFink May 12, 2023
f76ff55
spotless
ThomasAFink May 12, 2023
b2dfec6
spotless
ThomasAFink May 16, 2023
a1ddd79
fixed upload type
ThomasAFink May 16, 2023
8801488
fixed upload validation
ThomasAFink May 22, 2023
e35e9fa
add offers
ThomasAFink May 24, 2023
bd56dee
create, manipulate, and delete offer services with validation
ThomasAFink May 25, 2023
62f9fda
spotless
ThomasAFink May 25, 2023
1a8d86e
Merge remote-tracking branch 'origin/dev' into feature/KOB-66_angebot…
ThomasAFink May 25, 2023
d1f33e1
merged
ThomasAFink May 25, 2023
d203594
fixed tests
ThomasAFink May 25, 2023
55d59b2
change date format to string
ThomasAFink May 25, 2023
3ad06aa
add unit tests
ThomasAFink May 26, 2023
ca4bad4
removed backend bug
ThomasAFink May 30, 2023
3f71768
Merge branch 'dev' into feature/KOB-66_angebote_stadtweit
ThomasAFink May 30, 2023
8d7fd68
remove log
ThomasAFink May 31, 2023
5df3bbe
clean up for merge
ThomasAFink May 31, 2023
856efb1
change to date format
ThomasAFink May 31, 2023
b9a35bc
remove unused var
ThomasAFink May 31, 2023
cd37e9d
remove duplicate view
ThomasAFink May 31, 2023
e03b8f9
Update application.yml (#23)
ThomasAFink Sep 14, 2023
08f6134
enable logging (#24)
ThomasAFink Sep 14, 2023
ff7258c
Add token length logging (#25)
ThomasAFink Sep 14, 2023
a0a6dfc
Update UserDataResolver.java
ThomasAFink Sep 14, 2023
e9dcdf2
Merge branch 'dev' into feature/KOB-66_angebote_stadtweit
ThomasAFink Oct 20, 2023
8c3b54a
resolve merge conflicts
ThomasAFink Oct 20, 2023
b48ad2e
Update application.yml
ThomasAFink Nov 8, 2023
b57d464
Update UserDataResolver.java
ThomasAFink Nov 8, 2023
6dd4db1
Merge branch 'dev' into feature/KOB-66_angebote_stadtweit
ThomasAFink Apr 24, 2024
76f1330
spotless apply
ThomasAFink Apr 24, 2024
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ dist/
nbdist/
.nb-gradle/

.vscode/
2 changes: 2 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@
<dependency>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
<version>5.4.32.Final</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.liquibase</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import feign.jackson.JacksonDecoder;
import feign.jackson.JacksonEncoder;
import io.swagger.v3.oas.annotations.Hidden;
import java.util.TimeZone;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
Expand Down Expand Up @@ -37,6 +38,7 @@ public class MicroServiceApplication implements WebMvcConfigurer {

public static void main(String[] args) {
System.setProperty("org.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH", "true");
TimeZone.setDefault(TimeZone.getTimeZone("UTC"));
SpringApplication.run(MicroServiceApplication.class, args);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.net.URL;
import java.util.List;
import java.util.UUID;
import javax.annotation.Nullable;
import javax.persistence.CollectionTable;
import javax.persistence.Column;
import javax.persistence.ElementCollection;
Expand Down Expand Up @@ -54,6 +55,7 @@ public class ContactPoint {

// Add Image Link as a column
@Column(name = "image")
@Nullable
private URL image;
Comment on lines +58 to 59
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing URL validation and storage

The current implementation has several potential issues:

  1. No URL format validation
  2. URLs can become invalid over time
  3. No size/length constraints

Consider:

  1. Adding @URL validation annotation
  2. Adding length constraints
  3. Storing the image directly or using a more robust image management system
 @Column(name = "image")
 @Nullable
+@URL(message = "Invalid URL format")
+@Size(max = 2083, message = "URL exceeds maximum length")
 private URL image;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Nullable
private URL image;
@Column(name = "image")
@Nullable
@URL(message = "Invalid URL format")
@Size(max = 2083, message = "URL exceeds maximum length")
private URL image;


public ContactPoint(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import de.muenchen.kobit.backend.contactpoint.view.ContactPointView;
import de.muenchen.kobit.backend.links.service.LinkService;
import de.muenchen.kobit.backend.links.view.LinkView;
import de.muenchen.kobit.backend.validation.Validator;
import de.muenchen.kobit.backend.validation.ContactPointValidator;
import de.muenchen.kobit.backend.validation.exception.ContactPointValidationException;
import java.util.ArrayList;
import java.util.Collections;
Expand All @@ -32,15 +32,15 @@ public class ContactPointCreationService {
private final LinkService linkService;
private final CompetenceService competenceService;
private final AdminService adminService;
private final List<Validator> validators;
private final List<ContactPointValidator> validators;

ContactPointCreationService(
ContactPointRepository contactPointRepository,
ContactService contactService,
LinkService linkService,
CompetenceService competenceService,
AdminService adminService,
List<Validator> validators) {
List<ContactPointValidator> validators) {
this.contactPointRepository = contactPointRepository;
this.contactService = contactService;
this.linkService = linkService;
Expand All @@ -59,7 +59,7 @@ public ContactPointView createContactPoint(ContactPointView contactPointView)
throw new ResponseStatusException(
HttpStatus.BAD_REQUEST, "The User has not the needed permission!");
}
for (Validator validator : validators) {
for (ContactPointValidator validator : validators) {
validator.validate(contactPointView);
}
ContactPoint newContactPoint = createNewContactPoint(contactPointView);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import de.muenchen.kobit.backend.links.model.Link;
import de.muenchen.kobit.backend.links.service.LinkService;
import de.muenchen.kobit.backend.links.view.LinkView;
import de.muenchen.kobit.backend.validation.Validator;
import de.muenchen.kobit.backend.validation.ContactPointValidator;
import de.muenchen.kobit.backend.validation.exception.ContactPointValidationException;
import de.muenchen.kobit.backend.validation.exception.contactpoint.InvalidCompetenceException;
import de.muenchen.kobit.backend.validation.exception.contactpoint.InvalidContactPointException;
Expand All @@ -45,15 +45,15 @@ public class ContactPointManipulationService {
private final AdminService adminService;
private final RelevanceService relevanceService;

private final List<Validator> validators;
private final List<ContactPointValidator> validators;

ContactPointManipulationService(
ContactPointRepository contactPointRepository,
ContactService contactService,
LinkService linkService,
CompetenceService competenceService,
AdminService adminService,
List<Validator> validators,
List<ContactPointValidator> validators,
RelevanceService relevanceService) {
this.contactPointRepository = contactPointRepository;
this.contactService = contactService;
Expand Down Expand Up @@ -100,7 +100,7 @@ public ContactPointView updateContactPoint(ContactPointView contactPointView, UU
if (contactPointView.getLinks() == null) {
contactPointView.setLinks(Collections.emptyList());
}
for (Validator validator : validators) {
for (ContactPointValidator validator : validators) {
validator.validate(contactPointView);
}
if (!isUserAuthorized(contactPointView.getDepartments())) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package de.muenchen.kobit.backend.offer.api;

import de.muenchen.kobit.backend.offer.service.OfferCreationService;
import de.muenchen.kobit.backend.offer.service.OfferDeletionService;
import de.muenchen.kobit.backend.offer.service.OfferManipulationService;
import de.muenchen.kobit.backend.offer.service.OfferService;
import de.muenchen.kobit.backend.offer.view.OfferView;
import de.muenchen.kobit.backend.validation.exception.OfferValidationException;
import java.util.List;
import java.util.UUID;
import org.springframework.web.bind.annotation.*;

@RestController
@RequestMapping("/offers")
public class OfferController {

private final OfferService offerService;
private final OfferManipulationService manipulationService;
private final OfferCreationService creationService;
private final OfferDeletionService deletionService;

public OfferController(
OfferService offerService,
OfferManipulationService manipulationService,
OfferCreationService creationService,
OfferDeletionService deletionService) {
this.offerService = offerService;
this.manipulationService = manipulationService;
this.creationService = creationService;
this.deletionService = deletionService;
}

@GetMapping
public List<OfferView> getOfferList() {
return offerService.getOfferList();
}

@GetMapping("/{id}")
public OfferView getOfferById(@PathVariable UUID id) {
return offerService.findById(id);
}
Comment on lines +39 to +41
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle the case when an offer is not found

The getOfferById method does not handle the scenario where an offer with the specified UUID does not exist. This could lead to a NoSuchElementException or a similar unhandled exception, resulting in a 500 Internal Server Error.

Consider returning a 404 Not Found status when the offer is not found to provide a clearer response to the client.

Possible code change:

public OfferView getOfferById(@PathVariable UUID id) {
    return offerService.findById(id)
            .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Offer not found"));
}


@PostMapping
public OfferView createOffer(@RequestBody OfferView offerView) throws OfferValidationException {
return creationService.createOffer(offerView);
}
Comment on lines +44 to +46
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure OfferValidationException is properly handled

If OfferValidationException is thrown, it may result in a 500 Internal Server Error if not handled. Clients should receive a 400 Bad Request status when the input validation fails.

Consider adding an exception handler to translate OfferValidationException into an appropriate HTTP response.

Possible code addition:

// In a @ControllerAdvice class
@ExceptionHandler(OfferValidationException.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public ErrorResponse handleOfferValidationException(OfferValidationException ex) {
    // Build and return an error response
}

This ensures that clients receive meaningful feedback when validation errors occur.


@PutMapping("/{id}")
public OfferView updateOffer(@PathVariable UUID id, @RequestBody OfferView offerView)
throws OfferValidationException {
return manipulationService.updateOffer(offerView, id);
}
Comment on lines +49 to +52
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Validate consistency between path variable and request body in updateOffer

In the updateOffer method, there is a potential for inconsistency if the id provided in the path variable does not match the id within the OfferView object.

Consider enforcing that the id in the OfferView matches the path variable or setting it explicitly to prevent unintended updates.

Possible code change:

public OfferView updateOffer(@PathVariable UUID id, @RequestBody OfferView offerView)
        throws OfferValidationException {
    offerView.setId(id); // Ensure the IDs are consistent
    return manipulationService.updateOffer(offerView, id);
}


@DeleteMapping("/{id}")
public void deleteOffer(@PathVariable UUID id) {
deletionService.deleteOffer(id);
}
Comment on lines +55 to +57
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Return appropriate HTTP status when deleting a non-existent offer

The deleteOffer method does not check if the offer with the given UUID exists before attempting deletion. This could result in an unhandled exception if the offer is not found.

Consider verifying the existence of the offer and returning a 404 Not Found status if it doesn't exist.

Possible code change:

public void deleteOffer(@PathVariable UUID id) {
    if (!offerService.existsById(id)) {
        throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Offer not found");
    }
    deletionService.deleteOffer(id);
}

Ensure that offerService has an existsById method implemented.

}
65 changes: 65 additions & 0 deletions src/main/java/de/muenchen/kobit/backend/offer/model/Offer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package de.muenchen.kobit.backend.offer.model;

import de.muenchen.kobit.backend.offer.view.OfferView;
import java.net.URL;
import java.sql.Date;
import java.util.UUID;
import javax.annotation.Nullable;
import javax.persistence.*;
Comment on lines +5 to +8
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Modernize date handling and Jakarta EE annotations

Consider the following improvements:

  1. Replace java.sql.Date with java.time.LocalDate for better date handling and time zone management
  2. Update javax.* imports to jakarta.* as per Jakarta EE standards
-import java.sql.Date;
+import java.time.LocalDate;
-import javax.annotation.Nullable;
-import javax.persistence.*;
-import javax.validation.constraints.NotNull;
+import jakarta.annotation.Nullable;
+import jakarta.persistence.*;
+import jakarta.validation.constraints.NotNull;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import java.sql.Date;
import java.util.UUID;
import javax.annotation.Nullable;
import javax.persistence.*;
import java.time.LocalDate;
import java.util.UUID;
import jakarta.annotation.Nullable;
import jakarta.persistence.*;

import javax.validation.constraints.NotNull;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import org.hibernate.annotations.GenericGenerator;

@Setter
@Getter
@NoArgsConstructor
@AllArgsConstructor
@Entity
@Table(name = "offer")
ThomasAFink marked this conversation as resolved.
Show resolved Hide resolved
public class Offer {

@Id
@NotNull
@GeneratedValue(generator = "UUID")
@GenericGenerator(name = "UUID", strategy = "org.hibernate.id.UUIDGenerator")
private UUID id;

@Nullable
@Column(name = "start_date")
private Date startDate;

@Nullable
@Column(name = "end_date")
private Date endDate;
Comment on lines +30 to +36
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add date range validation

While the fields are properly mapped, consider adding validation to ensure endDate is after startDate when both are present. This can prevent invalid data entry.

Add this validation using a custom validator:

@AssertTrue(message = "End date must be after start date")
private boolean isDateRangeValid() {
    if (startDate == null || endDate == null) {
        return true;
    }
    return endDate.after(startDate);
}


@NotNull
@Column(length = 255)
private String title;

@NotNull
@Column(columnDefinition = "TEXT")
private String description;

@NotNull
@Column(name = "image_link", length = 2048)
private URL imageLink;
Comment on lines +38 to +48
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation and security measures

Consider adding:

  1. URL validation for imageLink using @URL annotation
  2. XSS protection for title and description fields
  3. Pattern validation for title to prevent special characters if needed
 @NotNull
 @Column(length = 255)
+@Pattern(regexp = "^[\\p{L}\\p{N}\\s.,!?-]*$")
 private String title;

 @NotNull
 @Column(columnDefinition = "TEXT")
+@SafeHtml
 private String description;

 @NotNull
 @Column(name = "image_link", length = 2048)
+@URL(protocol = "https")
 private URL imageLink;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@NotNull
@Column(length = 255)
private String title;
@NotNull
@Column(columnDefinition = "TEXT")
private String description;
@NotNull
@Column(name = "image_link", length = 2048)
private URL imageLink;
@NotNull
@Column(length = 255)
@Pattern(regexp = "^[\\p{L}\\p{N}\\s.,!?-]*$")
private String title;
@NotNull
@Column(columnDefinition = "TEXT")
@SafeHtml
private String description;
@NotNull
@Column(name = "image_link", length = 2048)
@URL(protocol = "https")
private URL imageLink;


public Offer(Date startDate, Date endDate, String title, String description, URL imageLink) {
this.startDate = startDate;
this.endDate = endDate;
this.title = title;
this.description = description;
this.imageLink = imageLink;
}

public OfferView toListView() {
return new OfferView(id, startDate, endDate, title, description, imageLink);
}

public OfferView toView() {
return new OfferView(id, startDate, endDate, title, description, imageLink);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package de.muenchen.kobit.backend.offer.repository;

import de.muenchen.kobit.backend.offer.model.Offer;
import java.util.List;
import java.util.UUID;
import org.springframework.data.jpa.repository.JpaRepository;

public interface OfferRepository extends JpaRepository<Offer, UUID> {

List<Offer> findAllByOrderByStartDateAsc();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package de.muenchen.kobit.backend.offer.service;

import de.muenchen.kobit.backend.admin.service.AdminService;
import de.muenchen.kobit.backend.offer.model.Offer;
import de.muenchen.kobit.backend.offer.repository.OfferRepository;
import de.muenchen.kobit.backend.offer.view.OfferView;
import de.muenchen.kobit.backend.validation.OfferValidator;
import de.muenchen.kobit.backend.validation.exception.OfferValidationException;
import java.util.List;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
public class OfferCreationService {

private final OfferRepository offerRepository;
private final AdminService adminService;
private final List<OfferValidator> validators;

public OfferCreationService(
OfferRepository offerRepository,
AdminService adminService,
List<OfferValidator> validators) {
this.offerRepository = offerRepository;
this.adminService = adminService;
this.validators = validators;
}

@Transactional
public OfferView createOffer(OfferView offerView) throws OfferValidationException {
// Perform validation using the registered validators
for (OfferValidator validator : validators) {
validator.validate(offerView);
}

// Check for admin rights or perform any admin-related operations here

Comment on lines +36 to +37
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implement admin rights verification in createOffer method

There is a placeholder comment indicating that admin rights checks should be performed, but the actual implementation is missing. It's important to verify that only users with appropriate admin privileges can create offers to ensure security and proper access control.

You can implement the admin rights check using the AdminService as follows:

     // Check for admin rights or perform any admin-related operations here
+    if (!adminService.isAdmin()) {
+        throw new AccessDeniedException("User does not have admin rights to create an offer");
+    }

Ensure that AccessDeniedException is appropriately imported from org.springframework.security.access.AccessDeniedException or the appropriate package you are using for access control.

Committable suggestion skipped: line range outside the PR's diff.

Offer newOffer = createNewOffer(offerView);
return new OfferView(
newOffer.getId(),
newOffer.getStartDate(),
newOffer.getEndDate(),
newOffer.getTitle(),
newOffer.getDescription(),
newOffer.getImageLink());
}

private Offer createNewOffer(OfferView offerView) {
return offerRepository.save(offerView.toOffer());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package de.muenchen.kobit.backend.offer.service;

import de.muenchen.kobit.backend.offer.repository.OfferRepository;
import java.util.UUID;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
public class OfferDeletionService {

private final OfferRepository offerRepository;

public OfferDeletionService(OfferRepository offerRepository) {
this.offerRepository = offerRepository;
}

@Transactional
public void deleteOffer(UUID id) {

offerRepository.deleteById(id);
}
Comment on lines +17 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and logging for robust deletion operations.

The current implementation has several areas for improvement:

  1. Add existence check before deletion
  2. Implement proper error handling
  3. Add logging for auditing purposes
  4. Add method documentation

Consider implementing these improvements:

+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
+ import org.springframework.http.HttpStatus;
+ import org.springframework.web.server.ResponseStatusException;

  @Service
  public class OfferDeletionService {
+     private static final Logger logger = LoggerFactory.getLogger(OfferDeletionService.class);
      private final OfferRepository offerRepository;

      // ... constructor ...

+     /**
+      * Deletes an offer by its ID.
+      *
+      * @param id The UUID of the offer to delete
+      * @throws ResponseStatusException if the offer doesn't exist
+      */
      @Transactional
      public void deleteOffer(UUID id) {
+         logger.debug("Attempting to delete offer with ID: {}", id);
+         
+         if (!offerRepository.existsById(id)) {
+             logger.warn("Attempt to delete non-existent offer with ID: {}", id);
+             throw new ResponseStatusException(
+                 HttpStatus.NOT_FOUND,
+                 String.format("Offer with ID %s not found", id)
+             );
+         }
+         
          offerRepository.deleteById(id);
+         logger.info("Successfully deleted offer with ID: {}", id);
      }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Transactional
public void deleteOffer(UUID id) {
offerRepository.deleteById(id);
}
@Transactional
/**
* Deletes an offer by its ID.
*
* @param id The UUID of the offer to delete
* @throws ResponseStatusException if the offer doesn't exist
*/
public void deleteOffer(UUID id) {
logger.debug("Attempting to delete offer with ID: {}", id);
if (!offerRepository.existsById(id)) {
logger.warn("Attempt to delete non-existent offer with ID: {}", id);
throw new ResponseStatusException(
HttpStatus.NOT_FOUND,
String.format("Offer with ID %s not found", id)
);
}
offerRepository.deleteById(id);
logger.info("Successfully deleted offer with ID: {}", id);
}

}
Loading
Loading