OM-456 - #69
Conversation
📝 WalkthroughResumen ejecutivoSe añaden columnas de latitud y longitud a la tabla Cambios
Diagramas de secuenciasequenceDiagram
participant Cliente as Cliente
participant Servicio as Servicio de Direcciones
participant BD as Base de Datos
participant Nominatim as API Nominatim
Cliente->>Servicio: createAddressService(payload con lat/lon)
Servicio->>Servicio: Validar address requerida
Servicio->>Servicio: Validar rango de coordenadas
Servicio->>Nominatim: getReverseGeocodedAddress(lat, lon)
Nominatim-->>Servicio: {city, region, postal_code}
Servicio->>BD: Crear registro con todos los campos
BD-->>Servicio: address_id
Servicio-->>Cliente: Dirección creada
rect rgba(200, 150, 100, 0.5)
Note over Cliente,Nominatim: Flujo de actualización
end
Cliente->>Servicio: updateAddressService(payload parcial)
Servicio->>Servicio: Validar que lat y lon sean ambos o ninguno
alt Contiene coordenadas
Servicio->>Servicio: Validar rango de coordenadas
Servicio->>Nominatim: getReverseGeocodedAddress(lat, lon)
Nominatim-->>Servicio: {city, region, postal_code}
end
Servicio->>BD: Actualizar registro
BD-->>Servicio: address_id
Servicio-->>Cliente: Dirección actualizada
Esfuerzo estimado de revisión🎯 4 (Complejo) | ⏱️ ~45 minutos Poema
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/modules/users/addresses/services/addresses.services.js (1)
130-141: Considerar validación explícita denull.
Number(null)devuelve0, lo que pasaría la validación de rango. Si un cliente envíalatitude: null, se interpretaría comolatitude: 0(coordenadas cerca de África occidental). Podría ser intencional, pero vale la pena verificar si este comportamiento es deseado.♻️ Opción para rechazar null explícitamente
const validateCoordinateField = (value, fieldName, min, max) => { + if (value === null || value === undefined) { + throw { + status: 400, + message: `${fieldName} invalida`, + }; + } + const parsedValue = Number(value); if (!Number.isFinite(parsedValue) || parsedValue < min || parsedValue > max) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/users/addresses/services/addresses.services.js` around lines 130 - 141, La función validateCoordinateField está tratando null como 0 porque Number(null) === 0; modifica validateCoordinateField para rechazar explícitamente null y undefined (y opcionalmente cadena vacía) antes de convertir: comprueba if (value === null || value === undefined) y lanza el mismo error incluyendo fieldName; luego continua con Number(value) y la validación Number.isFinite(parsedValue) && rango (min, max). Asegúrate de actualizar el mensaje/estatus donde se lanza el error en validateCoordinateField para manejar este caso explícito.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prisma/migrations/20260328222709_adding_lat_long_to_addresses/migration.sql`:
- Around line 1-3: La columna "longitude" en la migración ALTER TABLE
"Addresses" usa DECIMAL(10,8) que solo permite 2 dígitos enteros y no cubre
±180; actualizar la declaración en migration.sql para usar DECIMAL(11,8) para
"longitude" (mantener "latitude" como DECIMAL(10,8) si corresponde) y luego
regenerar la migración a partir del schema corregido para que el archivo de
migración y schema.prisma queden consistentes; busca las referencias a
"Addresses", "latitude" y "longitude" en esta migración para realizar el cambio.
In `@prisma/schema.prisma`:
- Around line 121-122: La precisión de `longitude` en la definición del modelo
(campo `longitude`) es insuficiente; cambia su tipo de Decimal de `Decimal(10,
8)` a `Decimal(11, 8)` para permitir valores en el rango -180..180 (mantén
`latitude` como `Decimal(10, 8)`); ubica y actualiza la declaración del campo
`longitude` en el archivo prisma/schema.prisma donde aparecen las líneas
`latitude` y `longitude` para aplicar la corrección.
In `@src/modules/users/addresses/services/addresses.services.js`:
- Around line 150-166: Wrap the fetch call to Nominatim with an
AbortController-based timeout (e.g., 5–10s) and clear the timer after
completion, so the request to `fetch` (using `params.toString()`) is aborted if
it exceeds the timeout; also wrap the await fetch and subsequent await
response.json() in a try/catch to convert network/abort errors and JSON parse
errors into controlled throws with meaningful status codes and messages (e.g.,
504 for timeout, 502 for upstream errors, 400/422 for invalid JSON), and
preserve the existing `response.ok` check to throw the current 502 when the HTTP
status is not OK.
---
Nitpick comments:
In `@src/modules/users/addresses/services/addresses.services.js`:
- Around line 130-141: La función validateCoordinateField está tratando null
como 0 porque Number(null) === 0; modifica validateCoordinateField para rechazar
explícitamente null y undefined (y opcionalmente cadena vacía) antes de
convertir: comprueba if (value === null || value === undefined) y lanza el mismo
error incluyendo fieldName; luego continua con Number(value) y la validación
Number.isFinite(parsedValue) && rango (min, max). Asegúrate de actualizar el
mensaje/estatus donde se lanza el error en validateCoordinateField para manejar
este caso explícito.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac41e473-348a-438b-8ce8-d2c022b1c43c
📒 Files selected for processing (3)
prisma/migrations/20260328222709_adding_lat_long_to_addresses/migration.sqlprisma/schema.prismasrc/modules/users/addresses/services/addresses.services.js
| -- AlterTable | ||
| ALTER TABLE "Addresses" ADD COLUMN "latitude" DECIMAL(10,8), | ||
| ADD COLUMN "longitude" DECIMAL(10,8); |
There was a problem hiding this comment.
Misma limitación de precisión para longitude.
Esta migración refleja el problema de precisión mencionado en schema.prisma. DECIMAL(10,8) solo permite 2 dígitos enteros, pero la longitud puede ser hasta ±180. Después de corregir el schema a Decimal(11, 8) para longitude, se deberá regenerar esta migración.
🐛 Corrección requerida después de actualizar el schema
-- AlterTable
ALTER TABLE "Addresses" ADD COLUMN "latitude" DECIMAL(10,8),
-ADD COLUMN "longitude" DECIMAL(10,8);
+ADD COLUMN "longitude" DECIMAL(11,8);📝 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.
| -- AlterTable | |
| ALTER TABLE "Addresses" ADD COLUMN "latitude" DECIMAL(10,8), | |
| ADD COLUMN "longitude" DECIMAL(10,8); | |
| -- AlterTable | |
| ALTER TABLE "Addresses" ADD COLUMN "latitude" DECIMAL(10,8), | |
| ADD COLUMN "longitude" DECIMAL(11,8); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prisma/migrations/20260328222709_adding_lat_long_to_addresses/migration.sql`
around lines 1 - 3, La columna "longitude" en la migración ALTER TABLE
"Addresses" usa DECIMAL(10,8) que solo permite 2 dígitos enteros y no cubre
±180; actualizar la declaración en migration.sql para usar DECIMAL(11,8) para
"longitude" (mantener "latitude" como DECIMAL(10,8) si corresponde) y luego
regenerar la migración a partir del schema corregido para que el archivo de
migración y schema.prisma queden consistentes; busca las referencias a
"Addresses", "latitude" y "longitude" en esta migración para realizar el cambio.
| latitude Decimal? @db.Decimal(10, 8) | ||
| longitude Decimal? @db.Decimal(10, 8) |
There was a problem hiding this comment.
La precisión de longitude es insuficiente para valores válidos.
Decimal(10, 8) permite 10 dígitos totales con 8 decimales, dejando solo 2 dígitos enteros (rango: -99.99999999 a 99.99999999). La longitud geográfica válida es de -180 a 180, por lo que valores como 120.5, -150.0 o 180.0 no se pueden almacenar.
Para longitud se necesita Decimal(11, 8) (3 dígitos enteros + 8 decimales). Para latitud, Decimal(10, 8) es correcto.
🐛 Corrección propuesta
- latitude Decimal? `@db.Decimal`(10, 8)
- longitude Decimal? `@db.Decimal`(10, 8)
+ latitude Decimal? `@db.Decimal`(10, 8)
+ longitude Decimal? `@db.Decimal`(11, 8)📝 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.
| latitude Decimal? @db.Decimal(10, 8) | |
| longitude Decimal? @db.Decimal(10, 8) | |
| latitude Decimal? `@db.Decimal`(10, 8) | |
| longitude Decimal? `@db.Decimal`(11, 8) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prisma/schema.prisma` around lines 121 - 122, La precisión de `longitude` en
la definición del modelo (campo `longitude`) es insuficiente; cambia su tipo de
Decimal de `Decimal(10, 8)` a `Decimal(11, 8)` para permitir valores en el rango
-180..180 (mantén `latitude` como `Decimal(10, 8)`); ubica y actualiza la
declaración del campo `longitude` en el archivo prisma/schema.prisma donde
aparecen las líneas `latitude` y `longitude` para aplicar la corrección.
| const response = await fetch( | ||
| `https://nominatim.openstreetmap.org/reverse?${params.toString()}`, | ||
| { | ||
| headers: { | ||
| "User-Agent": "BackendMarketplace/1.0", | ||
| Accept: "application/json", | ||
| "Accept-Language": "es", | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw { | ||
| status: 502, | ||
| message: "No se pudo obtener la ubicacion desde Nominatim", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js fetch AbortSignal.timeout support version
💡 Result:
Node.js added support for AbortSignal.timeout in version 17.3.0. This static method on AbortSignal, which creates an AbortSignal that aborts after a specified delay with a TimeoutError, was introduced via commit 62c4b4c in late 2021 and included in the v17.3.0 release. Since Node.js fetch (powered by undici) accepts any AbortSignal in its options, AbortSignal.timeout has been usable with fetch since this version. It works reliably in modern Node.js versions, though early implementations had some undici-related bugs that were fixed in subsequent updates.
Citations:
- 1: https://nodejs.org/api/globals.html
- 2: nodejs/node@62c4b4c85b
- 3: lib: add AbortSignal.timeout nodejs/node#40899
- 4: api.AbortSignal.timeout - node support in 17.3.0 mdn/browser-compat-data#15884
- 5: https://nodejs.org/en/blog/release/v17.3.0
🏁 Script executed:
# Find package.json to check Node.js version requirement
find . -name "package.json" -type f | head -5Repository: CrisNAC/BackendMarketplace
Length of output: 84
🏁 Script executed:
# Check for .nvmrc or other version files
fd -e nvmrc -e node-versionRepository: CrisNAC/BackendMarketplace
Length of output: 52
🏁 Script executed:
# Read the full implementation of the addresses service file
wc -l src/modules/users/addresses/services/addresses.services.jsRepository: CrisNAC/BackendMarketplace
Length of output: 132
🏁 Script executed:
# Check Node.js version requirement
cat package.json | grep -A 5 -B 5 "engines\|node"Repository: CrisNAC/BackendMarketplace
Length of output: 903
🏁 Script executed:
# Read the full function implementation (lines 143-207)
sed -n '143,207p' src/modules/users/addresses/services/addresses.services.jsRepository: CrisNAC/BackendMarketplace
Length of output: 1768
Agregá timeout y manejo de errores de red para la llamada a Nominatim.
La llamada a fetch no tiene timeout configurado y no captura errores de red. Si Nominatim no responde o hay problemas de conectividad, la petición puede quedar colgada indefinidamente. Además, response.json() puede lanzar una excepción si el contenido no es JSON válido, lo que resultaría en un error 500 no controlado.
🛡️ Propuesta con timeout y manejo de errores
+const NOMINATIM_TIMEOUT_MS = 5000;
+
const getReverseGeocodedAddress = async (latitude, longitude) => {
const params = new URLSearchParams({
format: "json",
lat: latitude.toString(),
lon: longitude.toString(),
});
- const response = await fetch(
- `https://nominatim.openstreetmap.org/reverse?${params.toString()}`,
- {
- headers: {
- "User-Agent": "BackendMarketplace/1.0",
- Accept: "application/json",
- "Accept-Language": "es",
- },
- }
- );
+ let response;
+ try {
+ response = await fetch(
+ `https://nominatim.openstreetmap.org/reverse?${params.toString()}`,
+ {
+ headers: {
+ "User-Agent": "BackendMarketplace/1.0",
+ Accept: "application/json",
+ "Accept-Language": "es",
+ },
+ signal: AbortSignal.timeout(NOMINATIM_TIMEOUT_MS),
+ }
+ );
+ } catch (error) {
+ throw {
+ status: 502,
+ message: "No se pudo conectar con el servicio de geocodificación",
+ };
+ }
if (!response.ok) {
throw {
status: 502,
message: "No se pudo obtener la ubicacion desde Nominatim",
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/users/addresses/services/addresses.services.js` around lines 150
- 166, Wrap the fetch call to Nominatim with an AbortController-based timeout
(e.g., 5–10s) and clear the timer after completion, so the request to `fetch`
(using `params.toString()`) is aborted if it exceeds the timeout; also wrap the
await fetch and subsequent await response.json() in a try/catch to convert
network/abort errors and JSON parse errors into controlled throws with
meaningful status codes and messages (e.g., 504 for timeout, 502 for upstream
errors, 400/422 for invalid JSON), and preserve the existing `response.ok` check
to throw the current 502 when the HTTP status is not OK.
2 participants
Summary by CodeRabbit
Notas de Lanzamiento