complited assigment

[gubber230]

No description provided.

[mateacademy-ai-mentor]

Review Summary

I’m requesting changes because several critical requirements are not met and they will break functionality or the checklist expectations.

Critical issues to fix:

• Random endpoint selects by a guessed numeric id (CharacterController.java lines ~27–29). This is incorrect and will often return 404. Requirement: pick a random row from the local DB. How to fix: implement a service method like getRandomCharacter() that uses count() + PageRequest.of(randomIndex, 1) or a repository native query ORDER BY RAND() LIMIT 1, and call it from the controller.
• Search endpoint uses equality (CharacterServiceImpl.findByName → CharacterRepository.findByName). Requirement: substring, case-insensitive search. How to fix: change repository to List<Character> findByNameContainingIgnoreCase(String name) and update the service to use it.
• Startup import is not idempotent (CharacterServiceImpl.saveAll + CharacterMapper ignores id). Requirement: fetch once without creating duplicates on restarts. How to fix: add Optional<Character> findByExternalId(Long externalId) to the repository and in saveAll upsert by externalId (update existing entity or skip), or set the existing id before saving.
• Exception handling: ExternalApiException discards message/cause (ExternalApiException.java). Requirement: meaningful startup failures. How to fix: implement constructor public ExternalApiException(String message, Throwable cause) { super(message, cause); }.

Minor but recommended:

• Improve Swagger docs to explicitly show response schemas and parameter docs; consider returning a DTO for a stable API contract.
• In CharacterExternalApiService: re-interrupt on InterruptedException and consider saving per page to reduce memory.

You’ve set up pagination and mappings well—once the above are addressed, the solution should meet the requirements. Good work so far; these changes will make it robust and compliant.

---

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

Don't assume external API id range; pick from stored records.

The new Random().nextLong(1, 827) usage embeds the external API's count/range into runtime logic. Instead, query the local DB for available records and choose one of them randomly. This avoids coupling controller behavior to the external API and meets the "work exclusively with the local database" requirement.

[mateacademy-ai-mentor]

Current methods match usages in code, but the interface should be more explicit about required behaviors (random selection from DB, idempotent import by externalId, and substring name search). Expressing these in method signatures improves clarity and helps ensure the implementation meets the task checklist.

[mateacademy-ai-mentor]

saveAll currently maps DTOs to entities and calls repository.saveAll, which will always insert new rows. This causes duplicate records if the startup import runs multiple times. Implement upsert logic here: for each CharacterDto, check repository for existing entity by externalId (add findByExternalId), then set the existing entity's id on the mapped model or update the existing entity's fields before saving so you perform an update rather than insert. This change is required to satisfy the "fetch once" / idempotency requirement.

[mateacademy-ai-mentor]

save(characterDto) uses mapper.internalToModel and repository.save(character). Because the mapper currently ignores id when mapping to the model (see CharacterMapper), saving will insert a new entity rather than update. If you want to support updates, either extend CharacterDto to include id and map it, or perform a lookup by externalId before save and set the model.id to the found entity's id so JPA updates the row instead of inserting a duplicate.

[mateacademy-ai-mentor]

findByName delegates to repository.findByName(name). The task requires returning all characters whose name contains the search string (substring match). Change repository to a containing query such as List<Character> findByNameContainingIgnoreCase(String name) or use a @Query with LIKE %:name% and update this method to call that. This will meet the required search semantics.

[mateacademy-ai-mentor]

Consider making saveAll transactional and/or process incoming DTOs in batches to handle large imports safely. Annotating the method with @Transactional or saving page-by-page (if fetch logic provides pages) will make the import more robust and consistent in face of partial failures.

[mateacademy-ai-mentor]

Review Summary

I’m requesting changes because there are critical blockers that break core requirements and the build:

• Critical: Search endpoint does exact-match instead of required case-insensitive substring match. In CharacterRepository (e.g., line where findByName(String name) is declared) and CharacterServiceImpl (method delegating to it), replace with a contains/ignore-case query such as findByNameContainingIgnoreCase(String name) and update the service/controller to use it. This is required by the spec and currently returns wrong results.
• Critical: Random endpoint selects by a fabricated id range (CharacterController, around the lines generating new Random().nextLong(1, 827) and calling getById). This does not select a random persisted record and can fail. Implement DB-backed random selection (e.g., repository.count() -> random offset -> fetch one with Pageable, or fetch all IDs and pick one) so the character is truly chosen from the local database.
• Critical: CI build fails due to Checkstyle errors on MapStruct-generated sources (target/generated-sources/annotations/.../CharacterMapperImpl.java). Configure the maven-checkstyle-plugin to exclude generated sources (e.g., exclude target/generated-sources/** or add suppressions) so the build passes. This is currently preventing tests from running.

Additional important fixes (not blockers but recommended):

• Fix ExternalApiException constructor to call super(message, cause) so errors retain diagnostics.
• Ensure startup loader is idempotent to avoid unique constraint violations on externalId (e.g., skip if data exists or upsert/deduplicate by externalId), and consider saving per page/batches. Add null checks when reading info().next() and restore interrupt status when catching InterruptedException.
• In MapperConfig, replace the placeholder implementationPackage = "<PACKAGE_NAME>.impl" with a real package or remove it.

You’ve got a solid structure and good use of MapStruct and DTOs. Addressing the three blockers above will align the app with the assignment requirements and unblock the build. Keep up the good work.

---

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

Issue (minor): implementationPackage is set to the placeholder "<PACKAGE_NAME>.impl". This looks like a leftover placeholder rather than a real package name. Consider replacing it with the actual package (e.g., mate.academy.rickandmorty.mapper.impl) or removing the attribute to let MapStruct use its default implementation package.

[mateacademy-ai-mentor]

Bug: The controller fabricates a random id using a hard-coded range and calls getById(randomId). This assumes internal DB ids are contiguous and aligned with that magic constant (827). Requirement: the character must be selected randomly from the characters persisted in the local database. Consider implementing random selection by querying the DB (e.g., repository.count() -> random offset -> fetch with Pageable limit 1, or fetch all IDs and pick one randomly) instead of generating a numeric id. This change ensures the returned id is the internal DB identifier as required.

[mateacademy-ai-mentor]

Consider providing a service-level method to support random selection from the database (for example, Optional<Character> findRandom() or List<Long> findAllIds() plus a getById). Relying on fabricated/random numeric IDs in controllers is fragile; exposing a DB-backed mechanism for selecting a random record makes fulfilling the random-character requirement robust. See CharacterController usage for context.

[mateacademy-ai-mentor]

Add Javadoc comments to each method to clarify expected behavior (e.g., whether saveAll is expected to be idempotent/upsert or to throw on duplicates). The startup loader and tests rely on saveAll semantics; documenting expected behavior helps prevent inconsistencies between service, repository and loader.

[mateacademy-ai-mentor]

Issue: saveAll maps all incoming DTOs and calls repository.saveAll(...) unconditionally. Because Character.externalId is declared unique, repeated runs or duplicate data can cause unique-constraint violations. Requirement: startup data load must avoid duplicate loads on the same application context. Consider making this method idempotent by deduplicating DTOs by externalId, checking which externalIds already exist (query repository for existing externalIds), and persisting only new records, or perform upsert logic. Alternatively ensure the loader avoids re-inserting when repository.count() > 0. Also consider saving per-page or in batches to reduce memory/transaction size.

[mateacademy-ai-mentor]

Recommendation: Mark mutating operations with @transactional to ensure atomicity and rollback on failures. Add @Transactional to save(...) and saveAll(...) (or annotate the class) so that saving multiple entities is wrapped in a transaction rather than relying on repository-level defaults.

[mateacademy-ai-mentor]

Bug relative to requirements: findByName delegates to repository.findByName(name) which performs an exact match. The task requires returning all characters whose name contains the search string (case-insensitive). Update the repository to use findByNameContainingIgnoreCase(String name) or define a custom query WHERE LOWER(name) LIKE LOWER(CONCAT('%', :name, '%')), and adjust the service method to reflect substring search behavior.

[mateacademy-ai-mentor]

Review Summary

I’m requesting changes because there are critical blockers that break required functionality and/or violate the public API contract.

Critical issues to fix:

• Random endpoint logic is incorrect (mixes external vs internal ids): In CharacterServiceImpl#getRandomCharacter you compute a random value using MAX(externalId) and then call repository.findById(randomId). This often returns nothing or a wrong row because internal id ≠ externalId. Replace with a DB-backed selection that guarantees an existing persisted row, e.g. count + pageable offset (repository.count() + findAll(PageRequest.of(offset, 1))) or add CharacterRepository#findRandomCharacter() via a native query (ORDER BY RAND()/RANDOM()) and use it. This is a functional blocker for /characters/random.
• API response shape must return externalId as a string: Both endpoints currently return the JPA entity directly, which serializes externalId as a number. The assignment requires externalId to be a string and to expose only the specified fields. Introduce a response DTO (id, externalId as String, name, status, gender) and map entities to this DTO in CharacterController for both random and search endpoints.
• MapStruct config placeholder: MapperConfig sets implementationPackage = "<PACKAGE_NAME>.impl", which is invalid and can break builds/CI or generated package paths. Remove this attribute or replace it with a real package (e.g., mate.academy.rickandmorty.mapper.impl).

Minor but recommended improvements (not blocking):

• In CharacterExternalApiService, restore interrupt status when catching InterruptedException, add null-safety around info().next(), and consider saving per page to reduce memory and allow partial progress.
• Clarify CharacterService#getById(Long id) semantics (it actually fetches by externalId). Consider renaming to getByExternalId.

You’ve done a solid job on startup import, pagination, idempotent upsert, and search. Address the above items and you’ll fully meet the requirements.

---

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

Critical: The implementationPackage is set to a placeholder string "<PACKAGE_NAME>.impl" which is not valid. This can cause MapStruct to generate sources into an unexpected package or path and may contribute to CI/Checkstyle failures. Remove this attribute or replace it with a real package name under your project (e.g., mate.academy.rickandmorty.mapper.impl).

[mateacademy-ai-mentor]

Critical: getRandomCharacter mixes externalId and internal id. The method calls repository.findMaxExternalId() to get the max externalId, generates a random long in that range, and then calls repository.findById(randomId) (which looks up by the entity primary key). This mismatch will often return no result (or the wrong result) and can throw EntityNotFoundException. Replace this approach with a DB-backed random selection (count + Pageable, DB native random query, or pick from existing ids). See repository methods and usage for context.

[mateacademy-ai-mentor]

Recommendation: use one of these approaches to pick a persisted entity for random selection:

• count + pageable: long count = repository.count(); int offset = random.nextInt((int) count); Character c = repository.findAll(PageRequest.of(offset, 1)).getContent().get(0);
• Add a repository native query: @Query(value = "SELECT * FROM characters ORDER BY RAND() LIMIT 1", nativeQuery = true) Optional<Character> findRandomCharacter(); then call that from the service.
  Either guarantees selecting a real persisted row and avoids id-mixing issues. (Do not use MAX(externalId) + random then findById.)

[mateacademy-ai-mentor]

Confusing semantics: getById(Long id) delegates to repository.findByExternalId(id). The method name suggests internal DB id but it actually returns by externalId. This naming mismatch makes code harder to reason about and likely contributed to the random-selection bug. Consider renaming to getByExternalId or changing the implementation to use repository.findById.

[mateacademy-ai-mentor]

Optional improvement: consider adding null checks or defensive handling around the dataset during import (e.g., empty lists). Also, if the dataset becomes large, consider batching the saveAll to avoid large transaction memory pressure. These are optimizations — not required by the spec but helpful for robustness.

[mateacademy-ai-mentor]

Review Summary

Educational approval: You’ve met the core requirements well — endpoints return the correct DTO (with externalId as String), mappings via MapStruct are accurate, and the upsert logic during import is solid. Nice use of DTOs instead of exposing entities, and good validation on request parameters.

Key improvements to address next:

• Critical (Application.java line ~21): Guard the startup import so tests/CI don’t call the external API. Use @Profile("!test"), or a property like import.enabled with false in src/test/resources/application.properties, or skip when DB already has data.
• CharacterExternalApiService: restore interrupt status when catching InterruptedException (Thread.currentThread().interrupt()), add null checks around info/next before pagination, and consider saving per page instead of buffering all results to reduce memory usage and improve resilience.
• CharacterRepository/ServiceImpl: handle empty DB for random selection — change repo to Optional<Character> or null-check in service and throw a clear EntityNotFoundException. Also consider a DB-agnostic random strategy (count + PageRequest) to avoid ORDER BY RAND() portability issues with H2.
• Minor: import @MapperConfig instead of using the fully qualified name; consider @NotBlank for externalId.

Overall, you’re very close — the functionality is in place. Tightening these guards and null-safety will make your solution robust for tests and CI. Great progress!

---

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Footnotes

1. Rate AI review example ↩

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

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

[mateacademy-ai-mentor]

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.

[mateacademy-ai-mentor]

saveAll upsert logic is implemented correctly: you fetch by externalId, map/create an entity and update fields before saving. This matches the requirement of preserving externalId and avoiding duplicates; ensure mapper.internalToModel and updateEntityFromDto handle all fields as expected (see CharacterMapper). For very large DTO lists, consider persisting by chunks to reduce memory usage.

[mateacademy-ai-mentor]

The @transactional annotation over saveAll is appropriate. Ensure this transactional boundary is what you want during startup import (it will wrap the whole batch). If you change to chunked saves, consider adjusting transactional scope accordingly.

[mateacademy-ai-mentor]

getById uses repository.findById(id) and throws EntityNotFoundException when missing. Note this method expects the internal DB id (not the externalId). If you need lookup by externalId, add a dedicated method name to avoid confusion.

[mateacademy-ai-mentor]

findByName correctly delegates to repository.findByNameContainingIgnoreCase(name) and maps to DTOs. The controller already enforces @notblank on the request parameter, so this implementation is consistent with the endpoint contract.

[mateacademy-ai-mentor]

getRandomCharacter calls repository.findRandomCharacter() and immediately maps it to DTO. Add a null-check: if repository returns null (empty table), throw EntityNotFoundException or return an appropriate response instead of allowing a NullPointerException.