Skip to content

Conversation

@esedArk
Copy link

@esedArk esedArk commented Oct 7, 2025

Implementación de la API REST de contactos en Laravel 11 usando Docker + PostgreSQL. Incluye CRUD completo, validaciones, manejo global de errores con formato JSON estándar y tests automáticos (PHPUnit con SQLite in-memory).

Se agregó colección Postman para validación manual y seeders de ejemplo. Proyecto listo para levantar con docker compose up -d y ejecutar migraciones vía php artisan migrate --seed.

Implementación de la API REST de contactos en Laravel 11 usando Docker + PostgreSQL.
Incluye CRUD completo, validaciones, manejo global de errores con formato JSON estándar y tests automáticos (PHPUnit con SQLite in-memory).

Se agregó colección Postman para validación manual y seeders de ejemplo.
Proyecto listo para levantar con docker compose up -d y ejecutar migraciones vía php artisan migrate --seed.
@height
Copy link

height bot commented Oct 7, 2025

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

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

Other comments (14)
  • README.md (215-215) There's an inconsistency in the Adminer port numbers. In lines 63 and 81, you mention port `8081`, but in line 215, you reference port `7780`. Please update to use a consistent port number.
  • README.md (183-183) The database name in the setup instructions (`ooptimo_api` on line 183) doesn't match what was mentioned earlier in the documentation (`contacts_db` on line 80). Please update to use a consistent database name.
  • README.md (168-169) The repository clone instructions in lines 168-169 contain placeholder values (`` and ``). Please update these with the actual repository information to make the instructions directly usable.
  • .env.example (3-3) The APP_KEY should not be hardcoded in the .env.example file. This file serves as a template and should not contain actual keys. Users should generate their own keys using `php artisan key:generate` when setting up the project.
    APP_KEY=
    
  • README.md (241-241) The date mentioned in line 241 (`Octubre 2025`) is in the future. Please update this to reflect the actual implementation date.
  • docker-compose.yml (44-47) Hardcoded database credentials in Adminer configuration should also use environment variables for consistency and security.
                ADMINER_DEFAULT_SERVER: pgsql
                ADMINER_DEFAULT_USERNAME: ${DB_USERNAME:-sail}
                ADMINER_DEFAULT_PASSWORD: ${DB_PASSWORD:-password}
                ADMINER_DEFAULT_DATABASE: ${DB_DATABASE:-ooptimo_api}
    
  • docker-compose.yml (26-27) The PostgreSQL port is directly exposed to the host machine without using an environment variable. This could lead to port conflicts if another PostgreSQL instance is running. Consider making it configurable:
            ports:
                - '${DB_PORT:-5432}:5432'
    
  • app/Exceptions/Handler.php (36-36) Using string matching (`str_contains`) to detect specific database errors is brittle and may break if the database error message format changes. Consider checking the error code instead or implementing a more robust approach to identify invalid ID formats.
  • app/Http/Controllers/Api/ContactController.php (33-43) There's an inconsistency in how models are resolved across methods. The `show()` and `destroy()` methods manually find the Contact model and handle exceptions, while the `update()` method uses Laravel's route model binding.

    Consider using route model binding consistently for all methods. For example:

    public function show(Contact $contact): JsonResponse
    {
        return $this->successResponse(
            new ContactResource($contact)
        );
    }
    

    And then handle the 404 responses globally using a custom exception handler for ModelNotFoundException.

  • app/Http/Controllers/Api/ContactController.php (17-21) The `index()` method is using `Contact::all()` which loads all contacts into memory at once. This could cause performance issues as your database grows.

    Consider implementing pagination:

    public function index(): JsonResponse
    {
        $contacts = ContactResource::collection(Contact::paginate());
        return $this->successResponse($contacts);
    }
    
  • app/Http/Controllers/Api/ContactController.php (54-63) Similar to the `show()` method, the `destroy()` method should use route model binding for consistency:
    public function destroy(Contact $contact): JsonResponse
    {
        $contact->delete();
        return $this->successResponse(null, 'Contact deleted successfully');
    }
    
  • storage/contacts_api.postman_collection.json (4-4) The `_postman_id` field contains what appears to be a placeholder value (`abcd1234-5678-efgh-ijkl-1234567890`). This ID should be unique for each Postman collection. Consider generating a proper UUID for this field or removing it entirely to let Postman generate one automatically when imported.
  • storage/contacts_api.postman_collection.json (18-18) The example request contains what appears to be a real phone number (`610180280`). For documentation and examples, it's better to use obviously fictional data like `123456789` to avoid any potential privacy concerns.
  • storage/contacts_api.postman_collection.json (20-20) All API endpoints are hardcoded to `http://localhost:8082`. Consider using Postman environment variables for the host and port (e.g., `{{base_url}}/api/contacts`) to make the collection more portable across different environments.

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +29 to +31
POSTGRES_DB: ooptimo_api
POSTGRES_USER: sail
POSTGRES_PASSWORD: password
Copy link

Choose a reason for hiding this comment

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

The PostgreSQL password is hardcoded in the docker-compose file. Consider using environment variables for sensitive credentials instead.

Suggested change
POSTGRES_DB: ooptimo_api
POSTGRES_USER: sail
POSTGRES_PASSWORD: password
POSTGRES_DB: ${DB_DATABASE:-ooptimo_api}
POSTGRES_USER: ${DB_USERNAME:-sail}
POSTGRES_PASSWORD: ${DB_PASSWORD:-password}

return $this->errorResponse('Invalid ID format. The ID must be a number.', 400);
}

return $this->errorResponse($e->getMessage(), 500);
Copy link

Choose a reason for hiding this comment

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

The default error response for unhandled exceptions exposes the raw exception message ($e->getMessage()) which could leak sensitive information in production. Consider using a more generic error message for production environments and only showing detailed errors in development.

Suggested change
return $this->errorResponse($e->getMessage(), 500);
if (config('app.debug')) {
return $this->errorResponse($e->getMessage(), 500);
}
return $this->errorResponse('An unexpected error occurred', 500);

## 🧩 Notas personales / Observaciones técnicas

Durante la implementación se utilizó **Laravel 11** con entorno **Dockerizado** mediante *Laravel Sail*, y base de datos **PostgreSQL 16**.
El contenedor principal expone el proyecto en el puerto `8080`, y se incluye un contenedor adicional de **Adminer** para inspeccionar fácilmente la base de datos (puerto `8081`).
Copy link

Choose a reason for hiding this comment

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

There's an inconsistency in the port numbers mentioned for the API. In line 63, you mention the project is exposed on port 8080, but in lines 119, 214, and 229, you reference port 8082 for the API. Please update to use consistent port numbers throughout the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant