Skip to content

complited assigment #264

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

complited assigment #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e797546
complited assigment
gubber230 Sep 27, 2025
5c06fd5
plugin fix
gubber230 Sep 27, 2025
df53150
fixed
gubber230 Sep 27, 2025
a507971
checkstyle fix
gubber230 Sep 27, 2025
718b5d6
fix2
gubber230 Sep 28, 2025
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
76 changes: 71 additions & 5 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
<name>jv-rick-and-morty</name>
<description>jv-rick-and-morty</description>
<properties>
<mapstruct.version>1.6.3</mapstruct.version>
<lombok-mapstruct-binding.version>0.2.0</lombok-mapstruct-binding.version>
<java.version>17</java.version>
<maven.checkstyle.plugin.version>3.1.1</maven.checkstyle.plugin.version>
<maven.checkstyle.plugin.configLocation>
Expand All @@ -32,6 +34,23 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mapstruct</groupId>
<artifactId>mapstruct</artifactId>
<version>${mapstruct.version}</version>
</dependency>

<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok-mapstruct-binding</artifactId>
<version>${lombok-mapstruct-binding.version}</version>
</dependency>
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>1.18.30</version>
</dependency>

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-data-jpa</artifactId>
Expand All @@ -41,14 +60,48 @@
<groupId>com.h2database</groupId>
<artifactId>h2</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
</dependency>
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>8.0.33</version>
</dependency>
<dependency>
<groupId>org.springdoc</groupId>
<artifactId>springdoc-openapi-starter-webmvc-ui</artifactId>
<version>2.2.0</version>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<source>${java.version}</source>
<target>${java.version}</target>
<annotationProcessorPaths>
<path>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>${lombok.version}</version>
</path>
<path>
<groupId>org.projectlombok</groupId>
<artifactId>lombok-mapstruct-binding</artifactId>
<version>${lombok-mapstruct-binding.version}</version>
</path>
<path>
<groupId>org.mapstruct</groupId>
<artifactId>mapstruct-processor</artifactId>
<version>${mapstruct.version}</version>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
Expand All @@ -66,8 +119,21 @@
<consoleOutput>true</consoleOutput>
<failsOnError>true</failsOnError>
<linkXRef>false</linkXRef>
<sourceDirectories>src/main</sourceDirectories>
</configuration>
</plugin>
<plugin>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
<configuration>
<excludes>
<exclude>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
</exclude>
</excludes>
</configuration>
</plugin>
</plugins>
</build>

Expand Down
13 changes: 12 additions & 1 deletion src/main/java/mate/academy/rickandmorty/Application.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
package mate.academy.rickandmorty;

import lombok.RequiredArgsConstructor;
import mate.academy.rickandmorty.service.CharacterExternalApiService;
import org.springframework.boot.CommandLineRunner;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;

@SpringBootApplication
public class Application {
@RequiredArgsConstructor
Copy link

Choose a reason for hiding this comment

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

Good: Using Lombok @requiredargsconstructor and a final service field ensures the external service is injected via constructor. This is the expected pattern for wiring the startup loader dependency.

public class Application implements CommandLineRunner {
Copy link

Choose a reason for hiding this comment

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

Suggestion: the class implements CommandLineRunner which is okay, but ensure the execution is guarded. If you keep CommandLineRunner, use one of the conditional approaches described in the previous comment. Alternatively, consider @eventlistener(ApplicationReadyEvent.class) — the lifecycle hook is different but still requires guarding for tests. The main goal is to avoid executing the external import during test contexts.

private final CharacterExternalApiService externalApiService;
Comment on lines +10 to +12
Copy link

Choose a reason for hiding this comment

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

Dependency injection via Lombok's @requiredargsconstructor is fine here. Ensure Lombok is present in project dependencies so the final constructor for externalApiService is generated correctly at compile time.


public static void main(String[] args) {
SpringApplication.run(Application.class, args);
Comment on lines 14 to 15
Copy link

Choose a reason for hiding this comment

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

Good: The class implements CommandLineRunner and provides a main() that calls SpringApplication.run(...). This is the correct way to start the Spring Boot application.


}

@Override
public void run(String... args) throws Exception {
Comment on lines +19 to +20
Copy link

Choose a reason for hiding this comment

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

Minor: run declares throws Exception and currently delegates the fetch call directly. Consider catching and handling import-related exceptions here (or in the service) so a transient external API failure doesn't fail the entire application startup silently during development. Also log useful diagnostics if import fails.

externalApiService.fetchCharacters();
Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

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

This class implements CommandLineRunner and calls externalApiService.fetchCharacters() in run(...). That correctly triggers the required startup data load once per application context creation (satisfies checklist item 1.3). Keep this behavior to meet the requirement.

Copy link

Choose a reason for hiding this comment

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

Caution: Because the fetch is unconditional, every application context creation (including test contexts) will attempt to call the external API. If you experience slow or flaky tests, consider making the startup fetch configurable (e.g., enabled by a property or active only in a specific profile). The task requires a fetch on context creation, so only add such guards if you also ensure tests still satisfy the assignment expectations. See the external API loader for idempotency improvements.

Copy link

Choose a reason for hiding this comment

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

Good: Calling externalApiService.fetchCharacters() here causes the application to fetch all characters on context creation, which fulfills the requirement that data must be loaded once at startup. Verify that CharacterExternalApiService.fetchCharacters() and CharacterServiceImpl.saveAll() remain idempotent to prevent duplicates on repeated starts.

Comment on lines +20 to +21
Copy link

Choose a reason for hiding this comment

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

Suggestion (optional): If you prefer the application to start even when the external API is temporarily unavailable, consider adding error handling/logging here to catch exceptions from fetchCharacters() and decide whether to continue startup or fail fast. Current behavior (propagating exceptions) may be acceptable if you want startup to fail on missing data.

Copy link

Choose a reason for hiding this comment

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

Critical: calling externalApiService.fetchCharacters() here will run on every ApplicationContext creation (including tests/CI) and will perform network calls to the external API. That makes tests brittle and slow. Make the startup import conditional (e.g. use a profile like @Profile("!test"), or guard with a property such as import.enabled and set it to false in src/test/resources/application.properties). This preserves "fetch once at startup" in normal runs while preventing external calls during tests. See CharacterExternalApiService.fetchCharacters() for the import behavior and the task description regarding startup import.

}
}
13 changes: 13 additions & 0 deletions src/main/java/mate/academy/rickandmorty/config/MapperConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package mate.academy.rickandmorty.config;

import org.mapstruct.InjectionStrategy;
import org.mapstruct.NullValueCheckStrategy;

@org.mapstruct.MapperConfig(
Copy link

Choose a reason for hiding this comment

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

Stylistic: line uses fully-qualified annotation name. Consider importing the annotation and using @MapperConfig instead of @org.mapstruct.MapperConfig for readability and consistency.

componentModel = "spring",
injectionStrategy = InjectionStrategy.CONSTRUCTOR,
nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS,
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

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

Suggestion: Keep the rest of the MapperConfig settings (componentModel, injectionStrategy, nullValueCheckStrategy). If you remove implementationPackage, MapStruct will use sensible defaults and generated classes will be placed next to the mapper implementations. After changing this, run the build and ensure generated sources are excluded from Checkstyle if needed.

implementationPackage = "mate.academy.rickandmorty.mapper.impl"
Copy link

Choose a reason for hiding this comment

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

Note: you set implementationPackage = "mate.academy.rickandmorty.mapper.impl". This is acceptable, but double-check that MapStruct is configured to generate implementations into that package and that generated sources are included in the build. If you don't need a custom package, removing this attribute avoids potential packaging mismatches.

)
public class MapperConfig {
}
40 changes: 40 additions & 0 deletions src/main/java/mate/academy/rickandmorty/controller/CharacterController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package mate.academy.rickandmorty.controller;

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import jakarta.validation.constraints.NotBlank;
import java.util.List;
import lombok.RequiredArgsConstructor;
import mate.academy.rickandmorty.dto.internal.CharacterDto;
import mate.academy.rickandmorty.service.CharacterService;
import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequiredArgsConstructor
@RequestMapping("/characters")
@Validated
public class CharacterController {
private final CharacterService characterService;

@Operation(description = "The request randomly generates a wiki "
+ "about one character in the universe the animated series Rick & Morty")
@GetMapping("/random")
Comment on lines +23 to +25
Copy link

Choose a reason for hiding this comment

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

Swagger docs are minimal; document responses/schema.

You have @Operation annotations (good), but to make endpoints discoverable/testable and to show the response model (fields id, externalId, name, status, gender) in OpenAPI UI, add explicit response annotations (e.g., @ApiResponse with response schema) or return a dedicated DTO annotated for OpenAPI. Also annotate the name parameter in the search endpoint so the UI shows how to call it. This addresses checklist items 1.5 and 3.7.

Comment on lines +23 to +25
Copy link

Choose a reason for hiding this comment

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

Good: Each endpoint has an @operation annotation, so they will appear in the OpenAPI/Swagger documentation as required by the task. Ensure your OpenAPI configuration (if any) is active so the endpoints are visible in Swagger UI.

public CharacterDto getRandomCharacter() {
return characterService.getRandomCharacter();
}

@Operation(description = "The request takes a string as an argument,"
+ " and returns a list of all characters whose name contains the search string.")
@GetMapping
public List<CharacterDto> getAllCharactersByName(
@Parameter(example = "?name=Rick+Sanchez")
@RequestParam("name")
@NotBlank
Comment on lines +34 to +36
Copy link

Choose a reason for hiding this comment

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

Good: The request parameter is validated with @notblank and an example is provided via @parameter. This satisfies the requirement to validate the presence of the search string (returns empty list when no matches — ensure service returns an empty list).

String name) {
return characterService.findByName(name);
Comment on lines +37 to +38
Copy link

Choose a reason for hiding this comment

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

The search endpoint returns a List (JPA entity) directly. Per the requirements, the response structure must include id, externalId (string), name, status, gender. Please map the entities to a response DTO that enforces these fields and types (especially making externalId a String) before returning the list.

}
}
8 changes: 8 additions & 0 deletions src/main/java/mate/academy/rickandmorty/dto/external/CharacterInfoDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package mate.academy.rickandmorty.dto.external;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;

@JsonIgnoreProperties(ignoreUnknown = true)
Copy link

Choose a reason for hiding this comment

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

Good: Using @JsonIgnoreProperties(ignoreUnknown = true) is appropriate — it makes the DTO resilient to additional fields from the external API.

Copy link

Choose a reason for hiding this comment

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

Good: The class uses @JsonIgnoreProperties(ignoreUnknown = true), which is appropriate for external API DTOs and prevents failures if the API returns unexpected fields.

public record CharacterInfoDto(
String next) {
Comment on lines +6 to +7
Copy link

Choose a reason for hiding this comment

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

Good: The record CharacterInfoDto(String next) correctly models the next pagination URL returned by the Rick & Morty API. This is exactly what's needed for the startup fetcher to follow paginated results.

Comment on lines +6 to +7
Copy link

Choose a reason for hiding this comment

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

Suggestion (optional): Add a short Javadoc above the record to explain the meaning of next (e.g., "URL of the next page; null if none"). This is purely documentation and not required for functionality.

Comment on lines +6 to +7
Copy link

Choose a reason for hiding this comment

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

Suggestion: The service calls info().next() during pagination. Consider that info could be null in unexpected responses — handle null-safety in the calling code (CharacterExternalApiService) to avoid a potential NullPointerException. The DTO itself is fine, but the caller should check info() before accessing next().

}
10 changes: 10 additions & 0 deletions src/main/java/mate/academy/rickandmorty/dto/external/CharacterResponseDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package mate.academy.rickandmorty.dto.external;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
Copy link

Choose a reason for hiding this comment

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

Ensure the project's Jackson version and configuration support Java record deserialization (Jackson 2.12+). CharacterExternalApiService uses ObjectMapper.readValue(...) to parse this record — without record support you may get deserialization errors at runtime 【}.

import java.util.List;

@JsonIgnoreProperties(ignoreUnknown = true)
Copy link

Choose a reason for hiding this comment

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

Good: Using @JsonIgnoreProperties(ignoreUnknown = true) is correct — it makes deserialization resilient to extra fields returned by the public API.

public record CharacterResponseDto(
CharacterInfoDto info,
Copy link

Choose a reason for hiding this comment

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

The info component can be null in malformed or unexpected responses. CharacterExternalApiService currently calls characterResponseDto.info().next() without null checks — consider validating info (and info.next) before use to avoid NullPointerException in the import flow 【}.

List<CharacterResultsDto> results) {
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

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

Good: The record fields CharacterInfoDto info and List<CharacterResultsDto> results correctly reflect the external API's paginated response shape and match the consumption in CharacterExternalApiService.

Copy link

Choose a reason for hiding this comment

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

The results field is correctly declared as a List. Keep @JsonIgnoreProperties(ignoreUnknown = true) enabled to tolerate additional fields from the external API.

}
11 changes: 11 additions & 0 deletions src/main/java/mate/academy/rickandmorty/dto/external/CharacterResultsDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package mate.academy.rickandmorty.dto.external;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;

@JsonIgnoreProperties(ignoreUnknown = true)
Copy link

Choose a reason for hiding this comment

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

@JsonIgnoreProperties(ignoreUnknown = true) is appropriate here to tolerate additional fields from the external API during deserialization. This prevents failures if the API adds fields in the future.

public record CharacterResultsDto(
Copy link

Choose a reason for hiding this comment

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

Declaration is correct: this record models a single result from the external API and matches the expected fields. Good use of a record for an immutable DTO. See CharacterResponseDto which uses a List for results.

String id,
String name,
String status,
String gender) {
}
12 changes: 12 additions & 0 deletions src/main/java/mate/academy/rickandmorty/dto/internal/CharacterDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package mate.academy.rickandmorty.dto.internal;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;

@JsonIgnoreProperties(ignoreUnknown = true)
Copy link

Choose a reason for hiding this comment

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

Minor: @JsonIgnoreProperties(ignoreUnknown = true) is safe and recommended for DTOs that are deserialized from an external API — it prevents failures when the API returns extra fields. No change required.

Copy link

Choose a reason for hiding this comment

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

Ensure your Jackson version supports Java record deserialization (Jackson 2.12+). CharacterExternalApiService uses ObjectMapper.readValue(...) to parse external responses into DTOs; if Jackson does not support records you may see runtime deserialization errors.

public record CharacterDto(
Copy link

Choose a reason for hiding this comment

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

Suggestion (optional): Add a short javadoc to state that externalId holds the original external API id and that callers should convert it to a string in API responses when required by the contract. This reduces confusion for maintainers.

Long id,
String externalId,
String name,
String status,
String gender) {
Comment on lines +6 to +11
Copy link

Choose a reason for hiding this comment

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

This record correctly defines the response fields and keeps externalId as a String, matching the assignment example and requirements.

Comment on lines +6 to +11
Copy link

Choose a reason for hiding this comment

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

Good: the app returns DTOs (not JPA entities) from controllers — verify CharacterController indeed returns CharacterDto to avoid exposing entity internals (controller already returns CharacterDto).

}
7 changes: 7 additions & 0 deletions src/main/java/mate/academy/rickandmorty/exception/ExternalApiException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package mate.academy.rickandmorty.exception;

public class ExternalApiException extends RuntimeException {
Copy link

Choose a reason for hiding this comment

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

This class currently has only a two-argument constructor with an empty body. You may also add convenience constructors (e.g., message-only, cause-only) or declare a serialVersionUID if you want explicit serialization control. The most important fix is to delegate to the superclass so thrown exceptions are informative during startup (Application -> fetchCharacters).

Copy link

Choose a reason for hiding this comment

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

Optional: you may add a serialVersionUID if you intend to serialize this exception, or add an additional message-only constructor for convenience. Neither is required for current usage.

Copy link

Choose a reason for hiding this comment

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

Optional: add a private static final long serialVersionUID for serialization stability (not required for normal use, but good practice if exceptions might be serialized).

public ExternalApiException(String message, Throwable cause) {
Copy link

Choose a reason for hiding this comment

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

Class is fine as-is. Consider adding convenience constructors (e.g., ExternalApiException(String message) and ExternalApiException(Throwable cause)) so callers can create the exception with only a message or only a cause when appropriate.

super(message, cause);
Comment on lines +4 to +5
Copy link

Choose a reason for hiding this comment

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

Constructor correctly passes the message and cause to the superclass, preserving the original exception and stack trace. This is the recommended behavior when wrapping exceptions.

Copy link

Choose a reason for hiding this comment

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

Note for callers: when wrapping InterruptedException in this exception (as CharacterExternalApiService currently does), restore the interrupt status with Thread.currentThread().interrupt() before throwing/wrapping to avoid swallowing the interrupt signal. See CharacterExternalApiService.fetchCharacters() where ExternalApiException is used.

}
}
33 changes: 33 additions & 0 deletions src/main/java/mate/academy/rickandmorty/mapper/CharacterMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package mate.academy.rickandmorty.mapper;

import java.util.List;
import mate.academy.rickandmorty.config.MapperConfig;
Copy link

Choose a reason for hiding this comment

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

Suggestion: You import and reference MapperConfig here. That config currently contains a placeholder implementationPackage = "<PACKAGE_NAME>.impl". Please remove or set a real package in MapperConfig to avoid unexpected MapStruct generation paths or CI/checkstyle problems. This change is in MapperConfig, not this file, but it directly affects generated mapper implementations.

import mate.academy.rickandmorty.dto.external.CharacterResultsDto;
import mate.academy.rickandmorty.dto.internal.CharacterDto;
import mate.academy.rickandmorty.model.Character;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.MappingTarget;
import org.mapstruct.Mappings;

@Mapper(config = MapperConfig.class)
public interface CharacterMapper {
CharacterDto toDto(Character character);
Copy link

Choose a reason for hiding this comment

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

toDto(Character character) returns CharacterDto which currently does not include the internal DB id. If you want MapStruct to produce API responses that include internal id, create a response DTO containing id (and externalId, name, status, gender) or extend CharacterDto to include id. Otherwise, ensure controllers return the JPA entity (as currently done) or add a dedicated response mapping for OpenAPI documentation purposes.


@Mapping(target = "id", ignore = true)
Character internalToModel(CharacterDto characterDto);
Comment on lines +17 to +18
Copy link

Choose a reason for hiding this comment

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

@mapping(target = "id", ignore = true) prevents mapping a primary key back into the model. Because CharacterDto doesn't carry an id, mapping always produces entities without an internal id and repository.saveAll will insert new rows. This makes startup ingestion non-idempotent unless saveAll performs dedup/upsert by externalId. Consider removing the ignore mapping and adding id to CharacterDto, or (recommended) implement upsert logic in the service layer that finds existing entities by externalId and sets their id before saving so records are updated rather than duplicated.

Comment on lines +17 to +18
Copy link

Choose a reason for hiding this comment

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

Correct: @Mapping(target = "id", ignore = true) on internalToModel prevents overwriting the DB-generated primary key when converting from DTO to entity. This is the right choice so that the internal id is managed by the database.

Comment on lines +17 to +18
Copy link

Choose a reason for hiding this comment

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

Good: internalToModel ignores the entity id while mapping from CharacterDto to Character, which is correct for creating/updating persisted entities (the database will generate the internal id).


@Mappings({
@Mapping(target = "externalId", source = "id"),
Comment on lines +20 to +21
Copy link

Choose a reason for hiding this comment

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

Good: Mapper is configured to map CharacterResultsDto.id to externalId, ensuring the externalId stays a String in DTOs and entities (this satisfies the requirement that externalId preserves the original external API id as a string).

@Mapping(target = "id", ignore = true)
})
CharacterDto externalToDto(CharacterResultsDto characterResultsDto);

@Mappings({
@Mapping(target = "externalId", source = "id"),
@Mapping(target = "id", ignore = true)
})
Comment on lines +20 to +29
Copy link

Choose a reason for hiding this comment

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

Suggestion: @Mappings({ ... }) is used to group mappings. Functionally fine, but you can use repeated @Mapping annotations on the method (MapStruct supports multiple @Mapping directly) for slightly clearer, modern syntax. This is a stylistic suggestion and not required.

List<CharacterDto> externalToDtoList(List<CharacterResultsDto> externalDtos);

void updateEntityFromDto(CharacterDto characterDto, @MappingTarget Character character);
Copy link

Choose a reason for hiding this comment

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

Positive: void updateEntityFromDto(CharacterDto characterDto, @MappingTarget Character character); is present and will be used for in-place updates (upsert behavior) during startup import. This supports idempotent loading and prevents duplicates when saving fetched pages. Good.

}
36 changes: 36 additions & 0 deletions src/main/java/mate/academy/rickandmorty/model/Character.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package mate.academy.rickandmorty.model;

import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
Copy link

Choose a reason for hiding this comment

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

Recommendation: Add a repository method like Optional<Character> findByExternalId(Long externalId) to support idempotent upsert operations during ingestion. The current CharacterRepository does not provide this helper, so adding it will simplify service implementation.

import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import lombok.Getter;
import lombok.Setter;
import lombok.ToString;

@Getter
@Setter
@ToString
@Table(name = "characters")
Copy link

Choose a reason for hiding this comment

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

Good: the table name is "characters", which matches the native query in CharacterRepository (SELECT * FROM characters ORDER BY RAND() LIMIT 1). This prevents table-name mismatches between entity and repository SQL.

@Entity
public class Character {
Copy link

Choose a reason for hiding this comment

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

Suggestion (optional): The simple class name Character collides with java.lang.Character. Consider renaming to CharacterEntity or similar to avoid confusion and accidental imports in code that deals with Character (the JDK type). This is a maintainability recommendation, not a functional blocker.

Copy link

Choose a reason for hiding this comment

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

The class name Character is valid but can be confused with java.lang.Character. Consider renaming to CharacterEntity or CharacterModel for clarity to future readers/imports.

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
Comment on lines +21 to +23
Copy link

Choose a reason for hiding this comment

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

Good: The entity defines an internal primary key id with @Id and @GeneratedValue, matching the requirement that id represents the internal DB identifier for Character entities.

Copy link

Choose a reason for hiding this comment

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

Optional: You currently expose the JPA entity in controllers. If you later return DTOs instead, ensure the DTO includes the internal id (the response example contains id) or map the entity id into the response DTO so the API response matches the required structure.

Comment on lines +20 to +23
Copy link

Choose a reason for hiding this comment

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

Model correctness: The entity defines the internal primary key id and required fields (externalId, name, status, gender) as specified in the requirements (checklist item 3.1). Good use of @GeneratedValue and validation annotations.

Comment on lines +21 to +23
Copy link

Choose a reason for hiding this comment

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

Correct: The primary key id is defined and generated by the database with GenerationType.IDENTITY, which satisfies the requirement for an internal DB-generated identifier.

@Column(nullable = false, unique = true)
@NotNull
private String externalId;
Comment on lines +25 to +26
Copy link

Choose a reason for hiding this comment

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

Consider using @notblank rather than @NotNull for externalId. That prevents empty strings (which are likely invalid) in addition to nulls. name, status, and gender already use @notblank.

@Column(nullable = false)
@NotBlank
private String name;
@Column(nullable = false)
@NotBlank
private String status;
@Column(nullable = false)
@NotBlank
private String gender;
Comment on lines +27 to +35
Copy link

Choose a reason for hiding this comment

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

Good: name, status, and gender are present and @NotBlank annotated, matching the response model fields required by the task (name, status, gender). Ensure these fields are populated when mapping from external DTOs so the API responses include them correctly.

Comment on lines +27 to +35
Copy link

Choose a reason for hiding this comment

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

Good: name, status, and gender are all non-nullable columns with @NotBlank validation, matching the assignment's required response fields.

}
16 changes: 16 additions & 0 deletions src/main/java/mate/academy/rickandmorty/repository/CharacterRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package mate.academy.rickandmorty.repository;

import java.util.List;
import java.util.Optional;
import mate.academy.rickandmorty.model.Character;
Copy link

Choose a reason for hiding this comment

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

Minor: importing and naming the entity as Character (mate.academy.rickandmorty.model.Character) works but shadows java.lang.Character and can be confusing. Consider renaming the entity class to avoid potential confusion during maintenance (optional).

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;

public interface CharacterRepository extends JpaRepository<Character, Long> {
Copy link

Choose a reason for hiding this comment

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

If you plan to support a convenient repository-level random selection for /characters/random, consider adding a DB-native method (MySQL):
@Query(value = "SELECT * FROM characters ORDER BY RAND() LIMIT 1", nativeQuery = true) Character findRandomCharacter();
Alternatively (portable), implement random selection in the service using long c = repository.count(); int idx = random.nextInt((int)c); repository.findAll(PageRequest.of(idx,1)).getContent().get(0);. The current controller logic of generating a random id in a fixed range is incorrect — choose one of the approaches above to pick an actual stored record.

List<Character> findByNameContainingIgnoreCase(String name);
Copy link

Choose a reason for hiding this comment

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

Good: findByNameContainingIgnoreCase(String name) implements a case-insensitive substring search directly in the DB. This matches the assignment requirement to search local DB records for names that contain the provided string (checklist item 1.2). Consider no changes here.


Optional<Character> findByExternalId(String externalId);
Copy link

Choose a reason for hiding this comment

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

Note: findByExternalId(String) correctly matches the entity's externalId type (String) and supports the upsert logic used during startup import (repository.findByExternalId(...) is used in saveAll). Ensure the DB unique constraint on externalId is present (it is in the entity) so finds are consistent.


@Query(value = "SELECT * FROM characters ORDER BY RAND() LIMIT 1", nativeQuery = true)
Copy link

Choose a reason for hiding this comment

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

High: the native query uses ORDER BY RAND(), which is fine for MySQL but can be brittle across different DBs/dialects (H2 used in tests). To make tests portable, either verify H2 supports this function in your test configuration or replace with a DB-agnostic approach (count + PageRequest offset) to select a random row.

Character findRandomCharacter();
Comment on lines +14 to +15
Copy link

Choose a reason for hiding this comment

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

Critical: findRandomCharacter() returns a raw Character. If the table is empty this will return null; CharacterServiceImpl currently calls mapper.toDto(repository.findRandomCharacter()) without null-check and will throw a NullPointerException. Change the repository method to return Optional or handle null in the service and throw a meaningful exception (e.g., EntityNotFoundException). See CharacterServiceImpl#getRandomCharacter usage for context.

}
56 changes: 56 additions & 0 deletions src/main/java/mate/academy/rickandmorty/service/CharacterExternalApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package mate.academy.rickandmorty.service;

import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.util.ArrayList;
import java.util.List;
import lombok.RequiredArgsConstructor;
import mate.academy.rickandmorty.dto.external.CharacterResponseDto;
import mate.academy.rickandmorty.dto.internal.CharacterDto;
import mate.academy.rickandmorty.exception.ExternalApiException;
import mate.academy.rickandmorty.mapper.CharacterMapper;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
public class CharacterExternalApiService {
private static final String BASE_URL = "https://rickandmortyapi.com/api/character";
private final ObjectMapper objectMapper;
private final CharacterMapper characterMapper;
private final CharacterService characterService;

public void fetchCharacters() {
Copy link

Choose a reason for hiding this comment

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

Optional approach to enforce "fetch once": instead of relying purely on upsert logic, you could check the repository (via characterService) at the start of fetchCharacters and skip fetching if characters already exist. This enforces a single import when DB is pre-populated. Be mindful of test setups where the DB may be empty and you do want to import (see Application.run).

List<CharacterDto> characters = new ArrayList<>();
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

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

Memory/use of batches: you collect all pages into characters (line 27) and then save them in one shot. For better memory usage and resilience, consider saving per page (call characterService.saveAll after mapping each characterResponseDto.results()), or use smaller batches.

Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

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

Accumulating all pages into a single List before saving can cause high memory usage and makes the load non-idempotent in practice. Consider saving per page or in batches and deduplicating by externalId, or implement upsert logic in the service layer so repeated runs won't cause unique constraint violations. This is relevant to the overall idempotency requirement.

String nextUrl = BASE_URL;

HttpClient client = HttpClient.newHttpClient();
while (nextUrl != null) {
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(nextUrl))
.build();
try {
HttpResponse<String> response = client.send(
request, HttpResponse.BodyHandlers.ofString());
if (response.statusCode() != 200) {
throw new IOException("API returned non-successful status code: "
+ response.statusCode());
}
Comment on lines +36 to +41
Copy link

Choose a reason for hiding this comment

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

Suggestion: consider adding basic retry/backoff or logging for non-200 responses (line 38-40) and set request timeouts to avoid long-hanging calls. This improves robustness against transient external API failures.

CharacterResponseDto characterResponseDto = objectMapper.readValue(
response.body(), CharacterResponseDto.class);
Comment on lines +42 to +43
Copy link

Choose a reason for hiding this comment

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

Correctness: ObjectMapper.readValue(...) deserializes into Java records. Ensure your project uses a Jackson version that supports record deserialization (Jackson 2.12+), otherwise this call may fail at runtime. If necessary configure the ObjectMapper or upgrade Jackson.


characters.addAll(characterMapper.externalToDtoList(
characterResponseDto.results()));
Comment on lines +31 to +46
Copy link

Choose a reason for hiding this comment

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

Good: the method implements paginated fetching by following the API info.next() link and maps external results to internal DTOs using CharacterMapper. That satisfies the requirement to use the public REST API and handle pagination. Consider batching saves per page for memory efficiency (see line 45 where you map results and line 53 where you saveAll).

nextUrl = characterResponseDto.info().next();
Copy link

Choose a reason for hiding this comment

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

Null-safety: nextUrl = characterResponseDto.info().next(); (line 47) assumes info() is non-null. While the API currently provides info, consider null-checking characterResponseDto.info() to avoid a potential NPE if the response shape changes or is malformed.

Comment on lines +45 to +47
Copy link

Choose a reason for hiding this comment

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

When advancing pages you call characterResponseDto.info().next() directly. If info() is null (or next() is null for last page), this may cause NPEs. Add a null-check for characterResponseDto.info() and handle a null next() safely. The CharacterResponseDto structure is used for deserialization, so validate its fields before using them.

Comment on lines +31 to +47
Copy link

Choose a reason for hiding this comment

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

Positive: The code correctly uses a pagination loop with nextUrl and sets it from characterResponseDto.info().next() to fetch all pages from the external API. This satisfies the requirement to fetch the full dataset at startup.

Comment on lines +42 to +47
Copy link

Choose a reason for hiding this comment

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

Recommendation: Add a null-safety check for characterResponseDto.info() before calling .next(). If info() is null (or results() is null), the current code will throw NPE. Example guard:

if (characterResponseDto.info() == null) {
    nextUrl = null;
} else {
    nextUrl = characterResponseDto.info().next();
}

This makes the loader robust against unexpected/malformed responses.

Copy link

Choose a reason for hiding this comment

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

Bug risk: nextUrl is set by characterResponseDto.info().next() without null checks. If info() or next() is null this will throw NPE. Add defensive checks for characterResponseDto.info() and characterResponseDto.info().next() before assigning nextUrl and decide how to handle (break loop or throw a descriptive exception).

} catch (IOException | InterruptedException e) {
throw new ExternalApiException(
"An error occurred while trying to get a response from the server.", e);
Comment on lines +48 to +50
Copy link

Choose a reason for hiding this comment

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

Exception wrapping bug downstream: you throw new ExternalApiException("An error...", e) (lines 48-50), but ExternalApiException's constructor doesn't call super(message, cause) and thus loses message and cause information. Fix ExternalApiException to call super(message, cause) so errors are informative and can be debugged (see ExternalApiException file).

Comment on lines +48 to +50
Copy link

Choose a reason for hiding this comment

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

InterruptedException handling: when catching InterruptedException you should restore the thread's interrupt status by calling Thread.currentThread().interrupt(); before throwing the runtime exception. This preserves the interrupt semantics for callers/containers.

Comment on lines +48 to +50
Copy link

Choose a reason for hiding this comment

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

Catching IOException | InterruptedException and wrapping into ExternalApiException is fine, but the ExternalApiException implementation currently discards the message and cause. Fix ExternalApiException to call super(message, cause); so the original exception and stack trace are preserved. This will make debugging much easier.

Comment on lines +48 to +50
Copy link

Choose a reason for hiding this comment

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

Important: The catch block lumps IOException and InterruptedException together and rethrows ExternalApiException, but does not restore the thread interrupt status when InterruptedException occurs. When an InterruptedException is caught you should call Thread.currentThread().interrupt() before throwing so the interrupt status is preserved. Consider catching InterruptedException separately and restoring the interrupt flag. Example:

} catch (InterruptedException ie) {
    Thread.currentThread().interrupt();
    throw new ExternalApiException("Interrupted while fetching characters", ie);
} catch (IOException e) {
    throw new ExternalApiException("IO error while fetching characters", e);
}

This change ensures correct thread behavior if the thread is interrupted.

Comment on lines +48 to +50
Copy link

Choose a reason for hiding this comment

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

Critical: InterruptedException is caught and wrapped but the thread interrupt status is not restored. When catching InterruptedException you should call Thread.currentThread().interrupt() before rethrowing or handling it to preserve interruption semantics.

}
Comment on lines +48 to +51
Copy link

Choose a reason for hiding this comment

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

When catching InterruptedException, consider restoring the thread interrupt status via Thread.currentThread().interrupt(); before rethrowing/wrapping, so the interrupt is not lost. Also consider adding retry/backoff for transient network errors (optional but recommended for robustness).

}
characterService.saveAll(characters);
Copy link

Choose a reason for hiding this comment

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

Idempotency issue: characterService.saveAll(characters) (line 53) will insert new entities for every run because current saveAll maps DTOs to models without checking existing externalId. This will create duplicates on repeated startups. Implement upsert logic in CharacterService.saveAll (find by externalId and set existing id before save) or check DB non-emptiness before fetching. See CharacterServiceImpl.saveAll for where to implement the fix .

Copy link

Choose a reason for hiding this comment

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

You call characterService.saveAll(characters) after collecting all DTOs. This unconditionally attempts to insert every record which may violate the unique constraint on externalId if duplicates exist. Make saveAll idempotent or deduplicate the list before saving, or persist per page and let the service do existence checks/upserts. Also consider saving in batches to avoid very large transactions.

Comment on lines +45 to +53
Copy link

Choose a reason for hiding this comment

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

Recommendation (performance/resilience): Currently all DTOs are accumulated in memory (characters.addAll(...)) and only saved once at the end with characterService.saveAll(characters). For large datasets or partial failures it is safer to persist per page: call characterService.saveAll(pageDtos) inside the loop after mapping each page. This reduces memory footprint and ensures partial progress is saved if the fetch fails later. If you prefer to keep a final save, consider at least flushing per N pages.

Copy link

Choose a reason for hiding this comment

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

Positive: Delegation to characterService.saveAll(characters) centralizes persistence and leverages the service's upsert logic (findByExternalId + update) to avoid duplicates on repeated starts. That's aligned with the assignment requirement for idempotent startup loading.

Comment on lines +26 to +53
Copy link

Choose a reason for hiding this comment

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

High: fetchCharacters performs network calls and collects all pages into memory before saving. This can make tests slow/fragile (if invoked during test context) and cause high memory usage for many pages. Prefer persisting per page inside the loop or flushing periodically rather than accumulating the entire dataset in memory. Also ensure Application startup invocation is guarded for tests (property/profile).

}

}
Loading