refactor: 좌표타입 geometry으로 변경 및 기반작업 #457
Conversation
…onLogs - getCronLogs 메소드에 key 파라미터를 추가하여 taskName을 기준으로 필터링 가능하도록 수정 - Cron 작업의 로그 태스크 이름을 간결하게 변경
…onLogs - getCronLogs 메소드에 key 파라미터를 추가하여 taskName을 기준으로 필터링 가능하도록 수정 - Cron 작업의 로그 태스크 이름을 간결하게 변경
# Conflicts: # src/modules/cron/cron.service.ts
# Conflicts: # src/modules/user/user.service.ts
# Conflicts: # src/common/guards/jwtAuth.guard.ts
# Conflicts: # package-lock.json # package.json
# Conflicts: # package-lock.json # package.json
# Conflicts: # eslint.config.js # package-lock.json # package.json
# Conflicts: # eslint.config.js
WalkthroughThe changes encompass modifications to multiple files, primarily enhancing documentation clarity, updating TypeScript linting configurations, and refining database schema and entity definitions. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EssayService
participant EssayRepository
participant ReviewService
User->>EssayService: Submit Essay
EssayService->>EssayRepository: Save Essay
EssayRepository-->>EssayService: Confirm Save
EssayService->>ReviewService: Check Status
ReviewService-->>EssayService: Return Status
EssayService-->>User: Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (17)
src/modules/essay/dto/essay.dto.ts (1)
Line range hint
1-75: Update API documentation to reflect coordinate changes.The API documentation should be updated to reflect the change from latitude/longitude to geometry type coordinates.
Add documentation for the coordinates field:
@ApiProperty({ description: 'Essay coordinates in WKT format (e.g., "POINT(longitude latitude)")', example: 'POINT(126.9779692 37.5662952)', })src/modules/essay/dto/request/createEssayReq.dto.ts (2)
Line range hint
71-75: Clean up commented code.There are commented-out lines related to array size validation (
@ArrayMaxSize). Either:
- Remove these comments if they're no longer needed
- Re-enable them if they're still required
- Add a comment explaining why they're temporarily disabled
Based on all the gathered information, I can now provide a final response:
Coordinate handling needs implementation for geometry conversion
The codebase shows proper setup for PostGIS with:
- Entity configured with geometry column using proper SRID (4326)
- Migration file that adds PostGIS extension and geometry column
- DTO maintaining backward compatibility with latitude/longitude fields
However, there's a critical gap:
- No visible implementation of the conversion from latitude/longitude to PostGIS geometry type
- Service layer only validates coordinate presence but doesn't show conversion logic
- No usage of PostGIS functions (ST_Point, ST_SetSRID) found in the service layer
Required actions:
- Implement coordinate conversion in the service layer using
ST_SetSRID(ST_Point(longitude, latitude), 4326)- Add conversion logic before saving the essay entity
- Consider adding a utility function for consistent coordinate handling
🔗 Analysis chain
Line range hint
43-57: Verify the coordinate handling strategy.The PR objectives indicate that coordinates should be stored as geometry type using PostGIS, but these fields still use separate latitude and longitude. While keeping the frontend request structure unchanged is good for backward compatibility, we need to ensure proper conversion to geometry type is happening in the service layer.
Questions to consider:
- How are these coordinates converted to WKT format for PostGIS storage?
- Should we add a comment documenting this conversion strategy?
Let's verify the coordinate handling in the service layer:
Let me gather more information about the coordinate handling by checking the entity and repository layers.
The previous results showed that the service layer validates coordinates but doesn't show the conversion logic. Let's examine how these coordinates are actually stored in the database through the entity and repository layers.
Let me try one more search with adjusted paths and patterns to find the entity definitions.
The previous searches didn't find the entity files in the expected location. Let's try to locate them first and then examine the coordinate handling implementation.
Let me check the essay entity and service implementation to understand the complete flow.
The previous results revealed that PostGIS is properly configured with a migration file and the entity has a geometry column. Let's see how the conversion from latitude/longitude to geometry is implemented in the service layer.
Let me check one more time specifically for the coordinate conversion logic in the service layer.
The entity shows proper PostGIS configuration, but we need to verify how the latitude/longitude from the DTO are converted to the geometry type in the service layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how coordinates are processed in the service layer # Expected: Find evidence of coordinate conversion to WKT/geometry type # Look for coordinate handling in essay service rg -A 10 "latitude.*longitude" "src/modules/essay/" # Check for PostGIS/geometry type usage rg -A 5 "geometry|ST_|Point|WKT" "src/modules/essay/"Length of output: 870
Script:
#!/bin/bash # Check essay entity and repository for geometry/coordinate handling # Look for column definitions and database interactions # Check essay entity rg -A 5 "location|coordinate|latitude|longitude|point|geometry" "src/modules/essay/entities/" # Check repository methods rg -A 5 "location|coordinate|latitude|longitude|point|geometry" "src/modules/essay/essay.repository.ts" # Check TypeORM column decorators for geometry type ast-grep --pattern '@Column({ $$$ type: $type, $$$ })'Length of output: 328
Script:
#!/bin/bash # Find essay-related entity files first fd "essay.*entity" # Then search for coordinate-related code in all TypeORM entities rg -A 5 "@Column.*(?:location|coordinate|latitude|longitude|point|geometry)" # Look for any database migration files that might show schema changes fd "migration|\.sql" # Check for any PostGIS-related configuration rg -A 5 "PostGIS|geography|geometry"Length of output: 2384
Script:
#!/bin/bash # Check essay entity implementation cat src/entities/essay.entity.ts # Check essay service implementation for coordinate conversion rg -B 5 -A 10 "coordinates.*=|createEssay" src/modules/essay/essay.service.tsLength of output: 4205
Script:
#!/bin/bash # Search for the createEssay method implementation and coordinate handling ast-grep --pattern 'async createEssay($$$) { $$$ }' # Check for any ST_* PostGIS function usage in the service rg -B 5 -A 10 "ST_Point|ST_SetSRID|ST_GeomFromText" src/modules/essay/ # Look for any utility functions that might handle coordinate conversion rg -B 5 -A 10 "convertToPoint|toGeometry|coordinates" src/modules/essay/Length of output: 203
src/modules/essay/dto/response/publicEssayRes.dto.ts (2)
Line range hint
1-9: Clean up unused importsThe
IsLatitudeandIsLongitudeimports are no longer used after commenting out the latitude and longitude properties.Apply this diff to remove unused imports:
import { IsArray, IsDateString, IsEnum, - IsLatitude, - IsLongitude, IsNumber, IsString, } from 'class-validator';
62-70: Consider adding documentation about temporary removalSince the removal of latitude/longitude fields is temporary (as per PR objectives), it would be helpful to add a TODO comment explaining why these fields are commented out and under what conditions they might be restored.
Apply this diff to add clarifying documentation:
+ // TODO: Latitude and longitude fields temporarily removed as part of migration to PostGIS geometry type. + // These fields may be restored in the future if needed for backward compatibility. // @ApiProperty() // @IsLatitude() // @Expose() // latitude: number; // // @ApiProperty() // @IsLongitude() // @Expose() // longitude: number;src/common/types/enum.types.ts (1)
44-44: Consider separating business logic changes into a dedicated PR.The addition of 'BURIED' status to both
EssayStatusandReviewQueueTyperepresents a significant business logic change. While these changes appear valid, they seem unrelated to the PR's main objective of refactoring coordinate storage to use geometry types.Consider:
- Moving these enum changes to a separate PR focused on the new essay status feature
- Linking that PR to any relevant feature requests or issues explaining the need for a 'buried' status
Also applies to: 65-65
src/modules/admin/dto/response/essayInfoRes.dto.ts (1)
39-45: Document the temporary removal of coordinate fields.While commenting out the fields aligns with the PR objectives of temporarily removing coordinates from the response, it would be helpful to add a TODO comment explaining:
- Why these fields are commented out
- When/under what conditions they might be restored
- How API consumers should handle the missing coordinates in the meantime
Apply this diff to improve documentation:
+ // TODO: Latitude and longitude fields temporarily removed as part of migration to PostGIS geometry type. + // These fields may be restored in the future if needed. Currently, coordinates are stored in WKB format + // in the database and clients should handle coordinate data through the new geometry-based endpoints. // @IsLatitude() // @Expose() // latitude: number; // // @IsLongitude() // @Expose() // longitude: number;src/modules/essay/dto/response/essayRes.dto.ts (2)
62-70: Remove commented code and document the change.Instead of keeping commented code, consider:
- Removing it entirely since it's tracked in version control
- Adding a comment explaining the transition to PostGIS geometry type
Replace the commented code with a documentation comment:
- // @ApiProperty() - // @IsLatitude() - // @Expose() - // latitude: number; - // - // @ApiProperty() - // @IsLongitude() - // @Expose() - // longitude: number; + /** + * Note: Latitude and longitude fields have been replaced with a PostGIS geometry field + * as part of the coordinate storage refactoring (#456). + * @see coordinates + */
Line range hint
1-13: Update imports after removing commented code.Once the commented code is removed, the
IsLatitudeandIsLongitudeimports will no longer be needed.import { IsArray, IsBoolean, IsDateString, IsEnum, - IsLatitude, - IsLongitude, IsNumber, IsOptional, IsString, } from 'class-validator';src/modules/admin/dto/response/fullEssayRes.dto.ts (1)
44-52: Remove commented code and clean up imports.Instead of keeping commented code, it should be removed since it's tracked in version control. Also, the unused imports
IsLatitudeandIsLongitudeshould be removed.Apply this diff:
import { IsBoolean, IsDate, IsDateString, IsEnum, - IsLatitude, - IsLongitude, IsNotEmpty, IsNumber, IsString, } from 'class-validator'; export class FullEssayResDto { // ... other properties ... - // @ApiProperty() - // @IsLatitude() - // @Expose() - // latitude: number; - // - // @ApiProperty() - // @IsLongitude() - // @Expose() - // longitude: number; // ... remaining properties ... }README.md (1)
76-76: Document PostGIS-related configuration requirements.While referring to the team channel for sensitive data is good practice, consider adding a non-sensitive example of PostGIS-related configuration that developers need to set up, such as:
Required PostGIS Configuration: - DATABASE_SRID=4326 # Spatial Reference System Identifier - DATABASE_EXTENSION=postgis # Required PostgreSQL extensionsrc/entities/essay.entity.ts (1)
41-47: Document the expected WKT format for coordinates.Since the coordinates field expects WKT format for input and returns WKB format, it's important to document this for other developers.
Add JSDoc comments to clarify the format:
+ /** + * Stores location coordinates in PostGIS geometry format + * @example Input: 'POINT(longitude latitude)' in WKT format + * e.g., 'POINT(127.1087183 37.3595316)' + * @returns WKB format when retrieved from database + */ @Column({ type: 'geometry', spatialFeatureType: 'Point', srid: 4326, // WGS 84 좌표계 nullable: true, }) coordinates: string;package.json (1)
Line range hint
1-156: Consider adding PostGIS type definitionsGiven that this PR introduces PostGIS geometry support, consider adding
@types/postgisas a dev dependency to provide better TypeScript type safety when working with geometry data types.Apply this diff to add PostGIS type definitions:
"devDependencies": { + "@types/postgis": "^3.3.3", // ... other dependencies }src/modules/essay/essay.controller.ts (1)
Line range hint
1-577: Consider adding geometry type information in API documentation.Given that this PR introduces geometry-based coordinate storage, it would be helpful to document the coordinate format in the API responses, particularly for endpoints that return essay data with coordinates.
Add WKB format information to relevant endpoint descriptions, for example:
@ApiOperation({ summary: '에세이 상세조회', description: ` + **Response Format:** + - Coordinates are returned in Well-Known Binary (WKB) format + - Clients need to convert WKB to WKT format if needed ...src/modules/essay/essay.repository.ts (1)
479-481: Consider using EssayStatus enum values directly.The change to use IN clause with specific statuses is more maintainable. Consider using enum values directly for better type safety.
.andWhere('essay.status IN (:...statuses)', { - statuses: [EssayStatus.PUBLISHED, EssayStatus.PRIVATE], + statuses: [EssayStatus.PUBLISHED, EssayStatus.PRIVATE] as EssayStatus[], });src/modules/essay/essay.service.ts (2)
405-405: Simplify null coalescing operator usage.The null coalescing operator usage can be simplified.
- : (aggregate.reputationScore ?? 0); + : aggregate.reputationScore ?? 0;
414-414: Enhance error logging.The error message could be more informative by including additional context.
- console.log(`${maxAttempts}번 시도 후 ${essay.id} 에세이에 대한 락을 획득하지 못했습니다.`); + console.log(`[updateEssayAggregateData] Failed to acquire lock for essay ${essay.id} after ${maxAttempts} attempts. Base retry delay: ${baseRetryDelay}ms`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
- README.md (3 hunks)
- eslint.config.js (2 hunks)
- package.json (1 hunks)
- src/common/types/enum.types.ts (2 hunks)
- src/entities/essay.entity.ts (1 hunks)
- src/migrations/2024/10/1729661076690-postgis.ts (1 hunks)
- src/modules/admin/dto/response/essayInfoRes.dto.ts (1 hunks)
- src/modules/admin/dto/response/fullEssayRes.dto.ts (1 hunks)
- src/modules/bury/bury.module.ts (1 hunks)
- src/modules/essay/dto/essay.dto.ts (1 hunks)
- src/modules/essay/dto/request/createEssayReq.dto.ts (1 hunks)
- src/modules/essay/dto/request/updateEssayReq.dto.ts (1 hunks)
- src/modules/essay/dto/response/essayRes.dto.ts (1 hunks)
- src/modules/essay/dto/response/publicEssayRes.dto.ts (1 hunks)
- src/modules/essay/essay.controller.ts (3 hunks)
- src/modules/essay/essay.repository.ts (4 hunks)
- src/modules/essay/essay.service.ts (6 hunks)
- src/modules/review/review.service.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/modules/bury/bury.module.ts
🧰 Additional context used
🔇 Additional comments (26)
src/modules/essay/dto/essay.dto.ts (1)
Line range hint
1-75: Verify frontend compatibility with the new coordinate format.While the PR states that frontend request structure remains unchanged, the removal of latitude/longitude validation might impact the frontend integration.
Let's verify the frontend usage:
✅ Verification successful
Let me try a different approach to search for frontend files and their usage patterns.
The previous search failed because of file type recognition. Let's try these alternative approaches to find relevant frontend code and coordinate usage patterns.
Let me search for more specific information about the coordinate changes and API usage.
This will help us understand how the coordinate system was changed and how it's being handled in the API layer.
Based on the code analysis, I can now provide a definitive response about the coordinate format changes:
Frontend compatibility is maintained despite coordinate format changes
The migration and codebase changes show that:
- The backend has transparently migrated from separate latitude/longitude fields to a PostGIS geometry type
- The coordinate handling is fully encapsulated in the backend:
- Input validation still checks for latitude/longitude in the request (
saveEssaymethod)- The conversion to PostGIS format happens internally
- The location string field remains unchanged for frontend display
No frontend changes are required as:
- The essay DTO still exposes the same interface
- The coordinate transformation is handled during save/update operations
- The location string field continues to be the primary display field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for latitude/longitude usage in frontend code rg -t ts -t tsx "latitude|longitude" # Search for coordinate-related API calls rg -t ts -t tsx "coordinates.*essay"Length of output: 133
Script:
#!/bin/bash # First, let's find frontend files that might contain these patterns fd -e js -e jsx -e ts -e tsx | grep -i "front\|client\|web" # Then search for any files containing essay-related API calls or models rg "essay" -l # Also check for any coordinate-related types or interfaces ast-grep --pattern 'interface $_ { $$$ latitude: $_ $$$ }' ast-grep --pattern 'interface $_ { $$$ longitude: $_ $$$ }'Length of output: 4685
Script:
#!/bin/bash # Check the essay entity and related migrations for coordinate changes cat src/entities/essay.entity.ts # Check the latest migration file that mentions postgis cat src/migrations/2024/10/1729661076690-postgis.ts # Look for location-related API calls in the essay controller cat src/modules/essay/essay.controller.ts # Check for any coordinate transformations in the essay service cat src/modules/essay/essay.service.tsLength of output: 47610
src/modules/essay/dto/request/updateEssayReq.dto.ts (1)
Line range hint
1-85: Add coordinates field to align with PR objectives.According to the PR objectives, the system is transitioning to use geometry type for coordinates storage using WKT format for input. However, this DTO lacks the coordinates field necessary for updating an essay's location.
Add the following field to support WKT coordinate input:
+ @ApiProperty({ + description: 'Location coordinates in WKT Point format (SRID: 4326)', + example: 'POINT(longitude latitude)', + required: false + }) + @IsString() + @IsOptional() + coordinates?: string;Let's verify if other DTOs have been updated with the coordinate field:
src/modules/review/review.service.ts (2)
27-28: LGTM! Clean implementation of the new status mapping.The new case for
EssayStatus.BURIEDfollows the existing pattern and maintains consistency with the method's structure.
27-28: Verify coordination with EssayService for buried essays.Since buried essays require coordinates (as per the AI summary), let's verify that this status mapping aligns with the coordinate validation in EssayService.
✅ Verification successful
Based on the results, I can now generate the final response since we have clear evidence of the coordination between ReviewService and EssayService for buried essays.
Coordination between services for buried essays is properly implemented
The code shows proper coordination between services:
- EssayService validates coordinates (latitude and longitude) when an essay's status is set to BURIED
- ReviewService correctly maps BURIED status to ReviewQueueType.BURIED
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coordination between ReviewService and EssayService for buried essays # Test 1: Check EssayService validation for BURIED status echo "Checking EssayService validation for BURIED status..." ast-grep --pattern 'if ($status === EssayStatus.BURIED && (!$latitude || !$longitude)) { $$$ }' # Test 2: Check for any other BURIED status handling echo "Checking other BURIED status handling..." rg -A 5 "EssayStatus.BURIED"Length of output: 1099
src/modules/essay/dto/request/createEssayReq.dto.ts (1)
39-39: LGTM: Status enum description update is accurate.The updated description correctly includes the new 'bury' status option.
src/common/types/enum.types.ts (1)
44-44: Verify and document the new BURIED status implementation.The addition of a 'BURIED' status represents a significant change to the essay lifecycle. Please ensure this new status is properly handled throughout the codebase.
Let's verify the implementation:
Consider adding JSDoc comments to document:
- The purpose of the BURIED status
- When an essay should transition to this status
- What actions are allowed on buried essays
eslint.config.js (2)
1-2: LGTM! Clean import declarations.The new imports improve code maintainability by centralizing TypeScript ESLint dependencies.
Line range hint
32-37: Verify TypeScript rules compatibility with geometry types.Given that this PR introduces geometry types, we should verify that the current TypeScript ESLint rules don't interfere with the new type usage.
✅ Verification successful
Based on the search results, I can now generate a final response since I have enough context about the geometry type usage in the codebase.
ESLint TypeScript rules are appropriately configured for geometry type usage
The current ESLint configuration is correctly set up to support the geometry type implementations found in the codebase:
- The disabled rules (
no-explicit-any,explicit-module-boundary-types, etc.) don't interfere with the PostGIS geometry type usage inessay.entity.ts- The geometry column definition and migrations show proper typing that works well with TypeScript's type system
- No TypeScript-specific patterns were found that would conflict with the current ESLint rules
The ESLint configuration actually helps by:
- Allowing explicit
anywhere needed for geometry type interactions- Not enforcing strict return types which can be helpful when working with PostGIS functions
- Permitting flexible module boundary types which is beneficial for geometry data handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing geometry type definitions and their usage rg -t ts "geometry|Point|4326|coordinates" --type-add 'ts:*.{ts,tsx}' # Check for any TypeScript-specific ESLint rule violations in files using geometry types ast-grep --pattern 'type $_ = geometry' -l tsLength of output: 1167
src/modules/admin/dto/response/essayInfoRes.dto.ts (1)
39-45: Verify impact on API consumers.The removal of latitude/longitude fields from the response DTO could impact existing API consumers.
Let's verify the usage of this DTO and potential impact:
✅ Verification successful
Let's examine the actual usage of these fields in the related DTOs and services to better understand the impact.
Removal of latitude/longitude fields is part of a coordinated database migration to PostGIS
Based on the evidence gathered, the removal of latitude/longitude fields from the DTOs is actually part of a planned database migration that converts these coordinates into a PostGIS geometry column. The migration file (1729661076690-postgis.ts) shows that:
- The existing latitude/longitude data is being preserved by converting it to PostGIS Point geometry:
UPDATE "essay" SET coordinates = ST_SetSRID(ST_MakePoint(longitude, latitude), 4326) WHERE latitude IS NOT NULL AND longitude IS NOT NULL;
The commented-out fields in multiple DTOs (PublicEssayResDto, EssayResDto, FullEssayResDto) align with this database change.
The essay service still validates coordinates for buried essays, suggesting the functionality is maintained but likely adapted for the new PostGIS format.
This appears to be a coordinated architectural change rather than an accidental removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct references to latitude/longitude in frontend code # and other potential consumers of this DTO # Search for any frontend code that might be accessing these properties rg -l "latitude|longitude" # Look for API documentation or OpenAPI/Swagger specs that might need updating fd -e yaml -e json . | xargs rg -l "latitude|longitude"Length of output: 497
Script:
#!/bin/bash # Check the content of related files to understand the usage patterns # and potential impact of removing these fields # Check the essay service implementation rg -A 5 -B 5 "latitude|longitude" src/modules/essay/essay.service.ts # Check the DTOs to understand the data flow echo "=== Public Essay Response DTO ===" cat src/modules/essay/dto/response/publicEssayRes.dto.ts echo "=== Essay Response DTO ===" cat src/modules/essay/dto/response/essayRes.dto.ts echo "=== Full Essay Response DTO ===" cat src/modules/admin/dto/response/fullEssayRes.dto.ts # Check the migration file to understand schema changes echo "=== Migration File ===" cat src/migrations/2024/10/1729661076690-postgis.tsLength of output: 10586
README.md (2)
94-94: LGTM!The Redis container setup is correct and unaffected by the coordinate storage changes.
107-111:⚠️ Potential issueRemove the synchronize:true workaround and document proper migration steps.
Using
synchronize: truein TypeORM is dangerous as it can lead to data loss, especially with the new PostGIS coordinate type. Instead, document the proper migration steps:### Database Setup Steps 1. Ensure PostGIS extension is installed 2. Run migrations to set up the coordinate column: ```bash npm run migration:run
- Verify the migration success by checking the Essay table schema
Let's verify if there are any migration files that handle the PostGIS setup: </blockquote></details> <details> <summary>package.json (1)</summary><blockquote> `111-112`: **LGTM: TypeScript ESLint updates** The version updates for `@typescript-eslint` packages are maintenance changes and look good. </blockquote></details> <details> <summary>src/modules/essay/essay.controller.ts (3)</summary><blockquote> `249-249`: **Documentation consistency with essay status filtering.** The documentation correctly includes 'BURY' in the list of excluded statuses for recommended essays, maintaining consistency with the new status handling. --- `279-279`: **Documentation consistency with essay status filtering.** The documentation correctly includes 'BURY' in the list of excluded statuses for following users' essays, maintaining consistency with the new status handling. --- `55-60`: **Documentation update aligns with new coordinate requirements.** The documentation correctly reflects the new requirements for essays with "buried" status, particularly the need for coordinate data. This aligns well with the PR's objective of transitioning to geometry-based coordinate storage. Let's verify the implementation of these requirements: <details> <summary>✅ Verification successful</summary> Let me try a different approach to verify the coordinate validation requirements. --- Let me try one more search to find the coordinate validation and buried status handling. --- **Documentation accurately reflects the implemented coordinate validation** Based on the code evidence found: - The essay service (`essay.service.ts`) properly validates coordinates for buried essays: ```typescript if (data.status === EssayStatus.BURIED) { if (!data.latitude || !data.longitude) throw new HttpException( '땅에 묻기 기능을 사용하기 위해선 좌표가 필요합니다.', HttpStatus.BAD_REQUEST, ); }
- The essay entity (
essay.entity.ts) uses PostGIS Point geometry type for coordinates storage:spatialFeatureType: 'Point', srid: 4326, // WGS 84 coordinate systemThe documentation's statement that "buried 요청의 경우 좌표 데이터가 필수로 필요합니다" (coordinates are required for buried requests) is correctly implemented in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the service layer validates coordinates for buried essays ast-grep --pattern 'if ($essay.status === $status) { $$$ coordinates $$$ }'Length of output: 83
Script:
#!/bin/bash # Search for coordinate validation in essay-related files echo "=== Searching for coordinate validation in service layer ===" rg -A 5 "coordinates.*buried" src/modules/essay/ echo "=== Checking essay status enum and buried status handling ===" rg -A 3 "enum.*EssayStatus" src/ rg -A 5 "case.*BURIED" src/ echo "=== Looking for coordinate validation in DTO ===" fd "dto" --type f --exec rg -l "coordinates"Length of output: 1298
Script:
#!/bin/bash # Search for SaveEssayDto and its coordinate validation echo "=== Checking SaveEssayDto definition ===" rg -A 10 "class.*SaveEssayDto" src/ echo "=== Looking for coordinate validation in essay service ===" rg -A 10 "async.*saveEssay" src/modules/essay/ echo "=== Checking Point type usage for coordinates ===" rg -A 3 "Point" src/Length of output: 5426
src/modules/essay/essay.repository.ts (3)
77-78: LGTM! Consistent status filtering.The addition of BURY status exclusion maintains consistency with the PR objectives and aligns with the overall filtering strategy.
118-119: LGTM! Comprehensive status filtering.The exclusion of both PRIVATE and BURY statuses is appropriate for the findTargetUserEssays method's purpose.
163-163: LGTM! Consistent recommendation filtering.The BURY status exclusion in getRecommendEssays maintains consistency with other query methods.
src/modules/essay/essay.service.ts (3)
158-158: LGTM: Message localization is correct.The error message is properly localized to Korean, maintaining consistency with other messages in the codebase.
161-163: LGTM: Improved query efficiency.Moving the story and tags fetching after permission checks is a good optimization. It prevents unnecessary database queries when the user doesn't have permission to perform the operation.
Line range hint
353-363: LGTM: Well-implemented exponential backoff.The implementation of exponential backoff for retry delays is a good practice. It helps prevent thundering herd problems and provides better distribution of retry attempts.
src/migrations/2024/10/1729661076690-postgis.ts (5)
31-33: Ensure PostGIS Extension Is Available in All EnvironmentsThe migration installs the PostGIS extension using:
CREATE EXTENSION IF NOT EXISTS postgis;Ensure that the database user has sufficient privileges to install extensions, and verify that all deployment environments (e.g., staging, production) support PostGIS. This prevents potential issues during deployment.
41-46: Verify Data Integrity During Coordinates MigrationThe UPDATE statement migrates existing
latitudeandlongitudedata into the newcoordinatescolumn using:UPDATE "essay" SET coordinates = ST_SetSRID(ST_MakePoint(longitude, latitude), 4326) WHERE latitude IS NOT NULL AND longitude IS NOT NULL;Ensure that all essays with non-null
latitudeandlongitudevalues are correctly transformed into thecoordinatesfield. This helps maintain data integrity during the migration.
49-52: Confirm Safe Removal of Latitude and Longitude ColumnsThe migration drops the
latitudeandlongitudecolumns:ALTER TABLE "essay" DROP COLUMN "latitude"; ALTER TABLE "essay" DROP COLUMN "longitude";Before dropping these columns, confirm that all necessary data has been successfully migrated to the
coordinatescolumn and that no application logic depends on the old columns.
64-66: Ensure Accurate Extraction of Coordinates in Down MigrationIn the
downmethod, the latitude and longitude are restored from thecoordinatescolumn:UPDATE "essay" SET latitude = ST_Y(coordinates), longitude = ST_X(coordinates);Verify that this reversal accurately restores the original
latitudeandlongitudevalues, considering any potential precision loss during the conversion.
78-99: Consistency in Enum Renaming and RestorationThe migration renames and restores enums for
essay_status_enumandreview_queue_type_enum. Ensure that the enum values are consistent across the application and that no application logic is broken due to these changes.
| // @IsLatitude() | ||
| // @Expose() | ||
| // latitude: number; | ||
| // | ||
| // @IsLongitude() | ||
| // @Expose() | ||
| // longitude: number; |
There was a problem hiding this comment.
Add validation for the new coordinate format.
While removing the latitude and longitude fields aligns with the migration to PostGIS geometry type, we should ensure proper validation for the new coordinate format. Consider adding validation for WKT format coordinates.
Add a new validator for the coordinates field:
@IsString()
@Matches(/^POINT\(\s*-?\d+\.?\d*\s+-?\d+\.?\d*\s*\)$/, {
message: 'Coordinates must be in WKT format (e.g., "POINT(longitude latitude)")',
})
@Expose()
coordinates: string;| @ApiProperty({ type: 'enum', description: 'private, published, linkedout' }) | ||
| @IsEnum(EssayStatus) | ||
| @IsOptional() | ||
| status?: EssayStatus; |
There was a problem hiding this comment.
Update status description to include all valid states.
The status description should be updated to include all valid states, including the new BURIED status mentioned in the enum changes.
- @ApiProperty({ type: 'enum', description: 'private, published, linkedout' })
+ @ApiProperty({ type: 'enum', description: 'private, published, linkedout, buried' })📝 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.
| @ApiProperty({ type: 'enum', description: 'private, published, linkedout' }) | |
| @IsEnum(EssayStatus) | |
| @IsOptional() | |
| status?: EssayStatus; | |
| @ApiProperty({ type: 'enum', description: 'private, published, linkedout, buried' }) | |
| @IsEnum(EssayStatus) | |
| @IsOptional() | |
| status?: EssayStatus; |
| // @ApiProperty() | ||
| // @IsLatitude() | ||
| // @Expose() | ||
| // latitude: number; | ||
| // | ||
| // @ApiProperty() | ||
| // @IsLongitude() | ||
| // @Expose() | ||
| // longitude: number; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now provide a final response:
Add coordinates field to expose PostGIS geometry data
The codebase shows clear evidence that:
- The essay entity uses PostGIS geometry type for coordinates storage (
type: 'geometry', spatialFeatureType: 'Point') - A migration exists that converts latitude/longitude to PostGIS Point geometry
- The entity defines a
coordinatesfield of type string
You should add the following to the DTO to expose the geometry data:
@ApiProperty()
@Expose()
coordinates: string;This aligns with:
- The entity's data model in
essay.entity.ts - The PostGIS migration that adds and uses the coordinates column
- The removal of individual latitude/longitude fields in favor of the geometry type
🔗 Analysis chain
Verify if coordinates field should be added
Since the system is now using PostGIS geometry type for coordinates storage, consider whether a coordinates field should be added to this DTO to expose the geometry data to API consumers.
Let's check how coordinates are handled in other DTOs:
Let me try a different approach to search for coordinates and geometry-related fields in the DTOs.
Let me check the actual implementation of these DTOs and related entity files to understand how coordinates are being handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other DTOs include a coordinates field
# and how they handle the geometry data
# Search for coordinates-related fields in DTOs
echo "Searching for coordinates field in DTOs..."
rg -l "coordinates" "src/modules/**/dto/**/*.ts"
# Search for geometry-related types or imports
echo "Searching for geometry type definitions..."
rg "geometry|Point|PostGIS" "src/modules/**/dto/**/*.ts"
Length of output: 431
Script:
#!/bin/bash
# Search for coordinates and geometry-related fields in all TypeScript files under src/modules
echo "Searching for coordinates field in DTOs..."
rg -g "*.dto.ts" "coordinates|geometry|Point|PostGIS" src/modules
# Look for latitude/longitude usage in DTOs to understand the pattern
echo -e "\nSearching for latitude/longitude usage..."
rg -g "*.dto.ts" "latitude|longitude" src/modules
# Find all DTO files to understand the structure
echo -e "\nListing all DTO files..."
fd ".*dto.ts$" src/modules
Length of output: 7442
Script:
#!/bin/bash
# Check essay entity and related files for geometry/coordinates implementation
echo "Checking essay entity implementation..."
rg -g "*.entity.ts" "geometry|Point|PostGIS|coordinates|latitude|longitude" src/modules
# Check if there are any migrations related to geometry/coordinates
echo -e "\nChecking database migrations..."
fd ".*migration.*ts$" src | xargs rg "geometry|Point|PostGIS|coordinates|latitude|longitude"
# Check the essay service implementation
echo -e "\nChecking essay service implementation..."
rg -g "*.service.ts" "geometry|Point|PostGIS|coordinates|latitude|longitude" src/modules/essay
Length of output: 3548
| '@typescript-eslint': configs.recommended, | ||
| prettier: require('eslint-plugin-prettier'), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consistent plugin import pattern.
While the TypeScript plugin now uses imported configs, there's an inconsistency in how plugins are configured (imported configs vs. direct requires).
Consider applying the same pattern to all plugins:
+const prettierPlugin = require('eslint-plugin-prettier');
plugins: {
'@typescript-eslint': configs.recommended,
- prettier: require('eslint-plugin-prettier'),
+ prettier: prettierPlugin,
},📝 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.
| '@typescript-eslint': configs.recommended, | |
| prettier: require('eslint-plugin-prettier'), | |
| }, | |
| const prettierPlugin = require('eslint-plugin-prettier'); | |
| '@typescript-eslint': configs.recommended, | |
| prettier: prettierPlugin, | |
| }, |
| parser: typescriptParser, | ||
| parserOptions: { | ||
| project: './tsconfig.json', | ||
| tsconfigRootDir: __dirname, | ||
| sourceType: 'module', | ||
| }, | ||
| globals: {}, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting parser options for better maintainability.
While the parser change is good, the configuration could be more maintainable by extracting the TypeScript-specific options.
Consider refactoring to:
+const tsParserOptions = {
+ project: './tsconfig.json',
+ tsconfigRootDir: __dirname,
+ sourceType: 'module',
+};
languageOptions: {
parser: typescriptParser,
- parserOptions: {
- project: './tsconfig.json',
- tsconfigRootDir: __dirname,
- sourceType: 'module',
- },
+ parserOptions: tsParserOptions,
},Committable suggestion was skipped due to low confidence.
| // @ApiProperty() | ||
| // @IsLatitude() | ||
| // @Expose() | ||
| // latitude: number; | ||
| // | ||
| // @ApiProperty() | ||
| // @IsLongitude() | ||
| // @Expose() | ||
| // longitude: number; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Coordinate field transition is properly handled across the codebase
The coordinate field removal strategy is well-implemented with:
- A PostGIS migration that properly handles the transition from latitude/longitude to geometry type
- Entity definition updated with the new coordinates field
- DTOs consistently commented out latitude/longitude fields
- Service layer validation updated for coordinates
The only remaining task is to update the createEssayReq.dto.ts which still has active latitude/longitude fields. This should be updated to use the new coordinates field format.
🔗 Analysis chain
Verify the coordinate field removal strategy.
The removal of latitude/longitude fields aligns with the PR objectives. However, let's verify that all related files are handling this transition consistently.
Let me gather more information to verify this properly.
Let me check the content of the remaining files that still reference latitude/longitude to ensure proper migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining latitude/longitude references in DTOs and entities
# that might need to be updated for consistency
echo "Checking for any remaining latitude/longitude references in DTOs:"
rg -l "latitude|longitude" "src/**/dto/"
echo -e "\nChecking for new coordinates field implementation in Essay entity:"
rg -A 5 "coordinates.*geometry" "src/entities/"
echo -e "\nChecking for PostGIS migration file:"
fd -g "*postgis*.ts" "src/migrations/"
Length of output: 608
Script:
#!/bin/bash
# Let's check the content of the PostGIS migration file and look for coordinate-related changes
echo "Checking PostGIS migration content:"
cat src/migrations/2024/10/1729661076690-postgis.ts
echo -e "\nChecking for Essay entity definition:"
fd -e ts "essay.entity" -x cat {}
echo -e "\nChecking for any remaining latitude/longitude references in the entire src directory:"
rg -l "latitude|longitude" "src/"
Length of output: 8062
Script:
#!/bin/bash
# Check the content of files still referencing latitude/longitude
echo "Checking essay service implementation:"
rg -A 5 "latitude|longitude" "src/modules/essay/essay.service.ts"
echo -e "\nChecking DTOs:"
for file in src/modules/essay/dto/**/*.dto.ts src/modules/admin/dto/**/*.dto.ts; do
if [ -f "$file" ]; then
echo -e "\n=== $file ==="
cat "$file"
fi
done
Length of output: 51628
| @@ -155,14 +84,14 @@ SERVICE_ACCOUNT_KEY_FILE= | |||
| ### 1. PostgreSQL 컨테이너 실행 | |||
| - 다음 명령어를 사용하여 PostgreSQL 컨테이너를 실행하세요. 각 항목의 설정을 환경변수에 등록해야하는걸 잊지마세요. | |||
| ```bash | |||
| docker run --name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgres:13 | |||
| docker run --name your_container_name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgres:13 | |||
There was a problem hiding this comment.
Update PostgreSQL container to include PostGIS extension.
Given the migration to PostGIS for coordinate storage, the PostgreSQL container needs the PostGIS extension. Update the command to use the PostGIS-enabled image:
-docker run --name your_container_name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgres:13
+docker run --name your_container_name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgis/postgis:13-3.3📝 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.
| docker run --name your_container_name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgres:13 | |
| docker run --name your_container_name -e POSTGRES_USER=your_db_user -e POSTGRES_PASSWORD=your_db_password -e POSTGRES_DB=your_db_name -p 5432:5432 -d postgis/postgis:13-3.3 |
| @Column({ | ||
| type: 'geometry', | ||
| spatialFeatureType: 'Point', | ||
| srid: 4326, // WGS 84 좌표계 | ||
| nullable: true, | ||
| }) | ||
| coordinates: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for WKT format.
To ensure data integrity, it would be beneficial to validate the WKT format before saving.
Consider adding a custom validator or using class-validator:
// Create a custom validator (in a separate file)
import { registerDecorator, ValidationOptions, ValidationArguments } from 'class-validator';
export function IsWKTPoint(validationOptions?: ValidationOptions) {
return function (object: Object, propertyName: string) {
registerDecorator({
name: 'isWKTPoint',
target: object.constructor,
propertyName: propertyName,
options: validationOptions,
validator: {
validate(value: any, args: ValidationArguments) {
if (!value) return true; // Allow null if nullable
const regex = /^POINT\(\s*-?\d+(\.\d+)?\s+-?\d+(\.\d+)?\s*\)$/;
return typeof value === 'string' && regex.test(value);
},
defaultMessage(args: ValidationArguments) {
return `${args.property} must be a valid WKT Point format (e.g., 'POINT(longitude latitude)')`;
},
},
});
};
}Then apply it to the coordinates field:
+ import { IsWKTPoint } from '../validators/wkt.validator';
@Column({
type: 'geometry',
spatialFeatureType: 'Point',
srid: 4326,
nullable: true,
})
+ @IsWKTPoint({ message: 'Invalid WKT Point format' })
coordinates: string;🛠️ Refactor suggestion
Add spatial index for the coordinates column.
For optimal performance with spatial queries, consider adding a spatial index to the coordinates column.
Add the spatial index using the @Index decorator:
+ @Index({ spatial: true })
@Column({
type: 'geometry',
spatialFeatureType: 'Point',
srid: 4326, // WGS 84 좌표계
nullable: true,
})
coordinates: string;📝 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.
| @Column({ | |
| type: 'geometry', | |
| spatialFeatureType: 'Point', | |
| srid: 4326, // WGS 84 좌표계 | |
| nullable: true, | |
| }) | |
| coordinates: string; | |
| @Index({ spatial: true }) | |
| @Column({ | |
| type: 'geometry', | |
| spatialFeatureType: 'Point', | |
| srid: 4326, // WGS 84 좌표계 | |
| nullable: true, | |
| }) | |
| coordinates: string; |
| if (data.status === EssayStatus.BURIED) { | ||
| if (!data.latitude || !data.longitude) | ||
| throw new HttpException( | ||
| '땅에 묻기 기능을 사용하기 위해선 좌표가 필요합니다.', | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Update coordinate validation to use geometry type.
The validation still uses separate latitude and longitude fields, but according to the PR objectives, we're transitioning to use geometry type for coordinates. This creates an inconsistency with the architectural changes.
Consider updating the validation to use the new geometry type:
- if (data.status === EssayStatus.BURIED) {
- if (!data.latitude || !data.longitude)
- throw new HttpException(
- '땅에 묻기 기능을 사용하기 위해선 좌표가 필요합니다.',
- HttpStatus.BAD_REQUEST,
- );
- }
+ if (data.status === EssayStatus.BURIED) {
+ if (!data.coordinates)
+ throw new HttpException(
+ '땅에 묻기 기능을 사용하기 위해선 좌표가 필요합니다.',
+ HttpStatus.BAD_REQUEST,
+ );
+ }Committable suggestion was skipped due to low confidence.
| // 4. PostGIS 확장 제거 (확장 제거는 필요 없을 시 생략 가능) | ||
| await queryRunner.query(` | ||
| DROP EXTENSION IF EXISTS postgis; | ||
| `); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a definitive response about the PostGIS dependencies.
Dropping PostGIS Extension Will Break Existing Functionality
The verification reveals active usage of PostGIS in the codebase:
- The
Essayentity actively uses PostGIS geometry type:@Column({ type: 'geometry', spatialFeatureType: 'Point', srid: 4326, nullable: true, })
- The migration itself shows that spatial data is being used for coordinates:
ADD COLUMN "coordinates" geometry(Point, 4326); SET coordinates = ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)
Dropping the PostGIS extension would break the existing spatial functionality in the Essay entity. The DROP EXTENSION statement should be removed from the migration as the extension is required for the application to function properly.
🔗 Analysis chain
Caution: Dropping PostGIS Extension May Affect Other Dependencies
The migration removes the PostGIS extension:
DROP EXTENSION IF EXISTS postgis;Dropping the PostGIS extension could impact other database objects that rely on it. Ensure that no other tables, functions, or extensions depend on PostGIS before executing this step.
Consider running the following script to identify dependencies:
Let me try a more comprehensive verification approach to check for PostGIS dependencies.
This comprehensive search will help us:
- Find any PostGIS-related code across the codebase
- Check other migrations that might be related to PostGIS
- Look for column type definitions in entities that could be using PostGIS types
The results will give us a better understanding of whether dropping the PostGIS extension could impact other parts of the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of PostGIS geometry types in the codebase.
# Expected Outcome: Identify any files that use PostGIS geometry types.
rg --files-with-matches 'geometry\(' src/entities/
Length of output: 52
Script:
#!/bin/bash
# Search for PostGIS-related patterns in the entire codebase
echo "=== Searching for PostGIS types and functions ==="
rg -i "postgis|geometry|geography|point|linestring|polygon|st_" -t ts -t sql
# Check for PostGIS-related database migrations
echo -e "\n=== Checking other migration files for PostGIS ==="
fd . src/migrations -e ts -x cat {} | grep -i "postgis"
# Look for type definitions that might use PostGIS types
echo -e "\n=== Checking TypeORM entity definitions ==="
ast-grep --pattern 'Column({
$$$
type: $_,
$$$
})'
Length of output: 8478
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/modules/essay/dto/saveEssay.dto.ts (1)
Line range hint
1-59: Consider adding documentation for coordinate handlingGiven the transition to PostGIS geometry type storage while maintaining latitude/longitude in the DTO, it would be helpful to add class-level JSDoc comments explaining:
- How coordinate data flows from DTO to storage
- Why latitude/longitude fields are maintained in the DTO
- Any transformation logic between DTO and storage formats
+/** + * Data Transfer Object for saving essays. + * Note: While the database uses PostGIS geometry type for coordinate storage, + * this DTO maintains separate latitude/longitude fields to preserve the existing API contract. + * Coordinate transformation to geometry type occurs in the service layer. + */ export class SaveEssayDto {src/modules/utils/utils.service.ts (1)
281-283: Add input validation and documentation for coordinate transformation.While the implementation correctly creates a PostGIS geometry point, it needs additional safeguards and documentation.
Consider applying these improvements:
+ /** + * Converts latitude and longitude to PostGIS geometry point in WKT format. + * @param latitude - Latitude in decimal degrees (-90 to 90) + * @param longitude - Longitude in decimal degrees (-180 to 180) + * @returns SQL expression creating a geometry point with SRID 4326 + * @throws {HttpException} If coordinates are invalid + * @example + * // Returns: ST_SetSRID(ST_GeomFromText('POINT(127.1087 37.3947)'), 4326) + * coordinatesToGeometry(37.3947, 127.1087) + */ coordinatesToGeometry(latitude: number, longitude: number): string { + if (latitude < -90 || latitude > 90) { + throw new HttpException('Invalid latitude: must be between -90 and 90', HttpStatus.BAD_REQUEST); + } + if (longitude < -180 || longitude > 180) { + throw new HttpException('Invalid longitude: must be between -180 and 180', HttpStatus.BAD_REQUEST); + } + if (latitude === null || longitude === null || isNaN(latitude) || isNaN(longitude)) { + throw new HttpException('Coordinates cannot be null or NaN', HttpStatus.BAD_REQUEST); + } return `ST_SetSRID(ST_GeomFromText('POINT(${longitude} ${latitude})'), 4326)`; }Key improvements:
- Added JSDoc documentation explaining the coordinate order and format
- Added input validation for coordinate ranges
- Added null/NaN checks
- Added example usage
src/modules/essay/essay.repository.ts (1)
197-197: Consider indexing optimization for complex filtering.While the BURIED status filter is correctly implemented, this method performs complex calculations and multiple joins. Consider adding a composite index to improve query performance.
Add the following index to your migration:
CREATE INDEX idx_essay_status_trend_score ON essay (status, trend_score) WHERE deleted_date IS NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/modules/bookmark/bookmark.controller.ts (1 hunks)
- src/modules/bookmark/bookmark.service.ts (1 hunks)
- src/modules/essay/dto/saveEssay.dto.ts (2 hunks)
- src/modules/essay/essay.repository.ts (5 hunks)
- src/modules/essay/essay.service.ts (7 hunks)
- src/modules/support/support.service.ts (1 hunks)
- src/modules/utils/utils.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/essay/essay.service.ts
🔇 Additional comments (10)
src/modules/essay/dto/saveEssay.dto.ts (3)
9-9: LGTM: Clean import additionsThe addition of ValidateNested and Type decorators is appropriate for handling nested tag validation.
Also applies to: 14-14
55-59: LGTM: Well-structured tag property implementationThe tags property is properly implemented with:
- Optional field annotation
- Nested validation for each Tag entity
- Proper type transformation setup
Line range hint
39-46: Verify: Should latitude/longitude fields be removed?According to the PR objectives, coordinates are being migrated to use PostGIS geometry type. However, these DTO fields still use separate latitude/longitude properties. While the PR mentions that frontend request structure remains unchanged, we should verify if this is intentional or if these fields should be replaced with a single coordinates field of geometry type.
✅ Verification successful
Let me gather more specific information about how coordinates are handled in the codebase to make a definitive assessment.
This will help us understand:
- How coordinates are stored in the database
- How the migration handles the transformation
- How coordinates are processed in the service layer
- How the repository layer handles the coordinate data flow
This information will allow us to determine if keeping latitude/longitude in DTOs is intentional for backward compatibility or if it should be updated.
Based on the analyzed code, I can now provide a definitive response about the latitude/longitude fields in the DTO.
Keep the latitude/longitude fields in DTOs for now
The codebase shows a clear pattern where:
- The database entity (
Essay) uses PostGIS geometry type for storing coordinates:@Column({ type: 'geometry', spatialFeatureType: 'Point', srid: 4326, nullable: true, }) coordinates: string;
- The repository layer handles the conversion from latitude/longitude to geometry in the
saveEssaymethod:coordinates: data.longitude && data.latitude ? () => `ST_SetSRID(ST_GeomFromText('POINT(${data.longitude} ${data.latitude})'), 4326)` : null,
- The
UtilsServicealso provides a helper method for this conversion:coordinatesToGeometry(latitude: number, longitude: number): string { return `ST_SetSRID(ST_GeomFromText('POINT(${longitude} ${latitude})'), 4326)`; }This architecture intentionally maintains separate latitude/longitude fields in DTOs while using PostGIS geometry type internally in the database. This is a good practice as it:
- Keeps the API interface simple and familiar for frontend clients
- Handles the geometry conversion in the backend
- Maintains backward compatibility
Therefore, these DTO fields should remain as they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other DTOs or entities have been updated to use geometry type # This will help understand if these fields should also be updated # Look for other files that might have coordinate-related changes rg -l "geometry|Point, 4326|coordinates" # Check for any remaining latitude/longitude usage rg -l "latitude|longitude"Length of output: 705
Script:
#!/bin/bash # Check the essay entity to see how coordinates are stored cat src/entities/essay.entity.ts # Check the migration file to understand the coordinate transformation cat src/migrations/2024/10/1729661076690-postgis.ts # Check utils service to see coordinate handling cat src/modules/utils/utils.service.ts # Check essay repository to understand data flow cat src/modules/essay/essay.repository.tsLength of output: 37948
src/modules/bookmark/bookmark.controller.ts (1)
68-69: Verify implementation of documented restrictionsThe API documentation now states two important restrictions:
- Users cannot bookmark their own essays
- Users cannot bookmark private essays
Let's verify if these restrictions are properly implemented in the service layer.
✅ Verification successful
Let me gather more specific information about the bookmark implementation.
The previous results showed some relevant code in the essay service about private essays, but we need to see the actual bookmark implementation to verify if these restrictions are enforced. Let's check the bookmark service implementation and any validation logic.
Restrictions are properly implemented in the service layer
The verification is complete. The restrictions mentioned in the API documentation are correctly implemented in the
BookmarkService.addBookmarkmethod:if (essay.status === EssayStatus.PRIVATE || essay.author.id === userId) throw new HttpException( '자신의 에세이 혹은 비공개 에세이는 북마크할 수 없습니다.', HttpStatus.BAD_REQUEST, );The code explicitly checks and prevents:
- Users from bookmarking their own essays (
essay.author.id === userId)- Users from bookmarking private essays (
essay.status === EssayStatus.PRIVATE)The error message in the implementation matches exactly with the documented restrictions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of these restrictions in the bookmark service echo "Searching for implementation of bookmark restrictions..." ast-grep --pattern 'async addBookmark($userId, $essayId) { $$$ }' # Search for related error messages echo "Searching for related error messages..." rg -A 5 "private.*essay|own.*essay" --type tsLength of output: 10631
Script:
#!/bin/bash # Search for the bookmark service implementation echo "Searching for bookmark service implementation..." ast-grep --pattern 'class BookmarkService { $$$ }' # Search for the specific addBookmark method echo "Searching for addBookmark method..." ast-grep --pattern 'async addBookmark($$$) { $$$ }' # Search for any validation or checks in the bookmark module echo "Searching for validation checks..." rg -A 10 "throw new HttpException" src/modules/bookmark/Length of output: 11624
src/modules/bookmark/bookmark.service.ts (2)
51-54: LGTM! Clear error message in Korean.The error message translation for invalid bookmark attempts (own essays or private essays) is clear and appropriate. The HTTP status code BAD_REQUEST (400) is correctly used for this validation error.
59-59: LGTM! Clear error message in Korean.The error message translation for duplicate bookmark attempts is clear and appropriate. The HTTP status code CONFLICT (409) is correctly used for this case.
src/modules/support/support.service.ts (1)
182-186: Verify the impact on device management flow.The new method creates devices without checking for duplicates, unlike
registerDevice. Please verify if this is intentional and document the use case for this separate flow.✅ Verification successful
Based on the search results, I can now provide a clear verification of the device management flows. Let me generate the final response:
Duplicate check is intentionally omitted in
newCreateDevice- different use case confirmedThe codebase shows two distinct device creation flows with different purposes:
registerDevice: Used in the support module for device registration with duplicate checking and FCM token managementnewCreateDevice: Used specifically in the essay module where a new device entry is needed without duplicate validationThe separate flow without duplicate checking is intentional as it serves a different use case in the essay service context. This is evidenced by its usage in
essay.service.tswhere a new device entry is required for essay data association.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to device creation methods to understand the different flows # Test: Search for both device creation methods. Expect: Different use cases in controllers/services. echo "Searching for device creation method calls..." rg -A 5 "newCreateDevice|registerDevice" --type ts echo "Searching for device-related controller endpoints..." ast-grep --pattern 'class $_ { $$$ @$_($$$) async $_($$$) { $$$device$$$ } $$$ }'Length of output: 6242
src/modules/essay/essay.repository.ts (3)
111-112: LGTM! Status filtering is properly implemented.The addition of BURIED status filter is correct and maintains consistency with existing filters.
152-153: LGTM! Status filtering is properly implemented.The addition of both PRIVATE and BURIED status filters enhances privacy and content management.
513-515: LGTM! Status filtering is more explicit and maintainable.The change to explicitly list allowed statuses (PUBLISHED, PRIVATE) improves code clarity and maintainability.
| coordinatesToGeometry(latitude: number, longitude: number): string { | ||
| return `ST_SetSRID(ST_GeomFromText('POINT(${longitude} ${latitude})'), 4326)`; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now generate a final response as there are clear patterns visible in the codebase.
Coordinate handling needs to be consolidated through the utility method
The verification reveals two key issues:
- The
coordinatesToGeometryutility method is not being consistently used, as evidenced by direct PostGIS point creation inessay.repository.ts - However, the coordinate order (longitude, latitude) is consistently maintained across all PostGIS point creations, including:
src/modules/utils/utils.service.ts:POINT(${longitude} ${latitude})src/modules/essay/essay.repository.ts:POINT(${data.longitude} ${data.latitude})- Migration file:
ST_MakePoint(longitude, latitude)
To maintain consistency and prevent future coordinate order confusion:
- Replace direct PostGIS point creation in
essay.repository.tswithcoordinatesToGeometryutility method - Keep the migration scripts as is since they're one-time operations
🔗 Analysis chain
Verify consistent coordinate handling across the codebase.
Let's ensure this utility method is used consistently and coordinates are handled in the correct order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential coordinate handling issues
# Test 1: Find all usages of coordinatesToGeometry
echo "=== Searching for coordinatesToGeometry usage ==="
rg "coordinatesToGeometry" -A 2
# Test 2: Look for potential coordinate order confusion
echo -e "\n=== Checking for potential coordinate order confusion ==="
rg -i "latitude.*longitude|longitude.*latitude" --type ts
# Test 3: Check for any remaining direct PostGIS point creation
echo -e "\n=== Checking for direct PostGIS point creation ==="
rg "ST_GeomFromText.*POINT" --type ts
Length of output: 2265
| await this.redis.del(`user:${user.id}`); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting Redis cache invalidation to a reusable method.
The Redis cache invalidation logic appears in multiple methods (e.g., registerDevice and newCreateDevice). Consider extracting this into a private method to maintain DRY principles and ensure consistent cache management.
+ private async invalidateUserCache(userId: number): Promise<void> {
+ await this.redis.del(`user:${userId}`);
+ }
async newCreateDevice(user: User, device: DeviceDto) {
const newDevice = await this.supportRepository.createDevice(user, {
os: device.os as DeviceOS,
type: device.type as DeviceType,
model: device.model,
});
- await this.redis.del(`user:${user.id}`);
+ await this.invalidateUserCache(user.id);
return await this.supportRepository.saveDevice(newDevice);
}📝 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.
| await this.redis.del(`user:${user.id}`); | |
| private async invalidateUserCache(userId: number): Promise<void> { | |
| await this.redis.del(`user:${userId}`); | |
| } | |
| async newCreateDevice(user: User, device: DeviceDto) { | |
| const newDevice = await this.supportRepository.createDevice(user, { | |
| os: device.os as DeviceOS, | |
| type: device.type as DeviceType, | |
| model: device.model, | |
| }); | |
| await this.invalidateUserCache(user.id); | |
| return await this.supportRepository.saveDevice(newDevice); | |
| } |
|
|
||
| await this.redis.del(`user:${user.id}`); | ||
|
|
||
| return await this.supportRepository.saveDevice(newDevice); | ||
| } |
There was a problem hiding this comment.
Add transaction management and input validation.
The newCreateDevice method should:
- Use the
@Transactional()decorator to ensure atomic operations - Validate the device DTO fields before processing
+ private validateDeviceDto(device: DeviceDto): void {
+ if (!Object.values(DeviceOS).includes(device.os)) {
+ throw new HttpException('Invalid device OS', HttpStatus.BAD_REQUEST);
+ }
+ if (!Object.values(DeviceType).includes(device.type)) {
+ throw new HttpException('Invalid device type', HttpStatus.BAD_REQUEST);
+ }
+ if (!device.model) {
+ throw new HttpException('Device model is required', HttpStatus.BAD_REQUEST);
+ }
+ }
+ @Transactional()
async newCreateDevice(user: User, device: DeviceDto) {
+ this.validateDeviceDto(device);
const newDevice = await this.supportRepository.createDevice(user, {
os: device.os as DeviceOS,
type: device.type as DeviceType,
model: device.model,
});
await this.redis.del(`user:${user.id}`);
return await this.supportRepository.saveDevice(newDevice);
}📝 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.
| await this.redis.del(`user:${user.id}`); | |
| return await this.supportRepository.saveDevice(newDevice); | |
| } | |
| private validateDeviceDto(device: DeviceDto): void { | |
| if (!Object.values(DeviceOS).includes(device.os)) { | |
| throw new HttpException('Invalid device OS', HttpStatus.BAD_REQUEST); | |
| } | |
| if (!Object.values(DeviceType).includes(device.type)) { | |
| throw new HttpException('Invalid device type', HttpStatus.BAD_REQUEST); | |
| } | |
| if (!device.model) { | |
| throw new HttpException('Device model is required', HttpStatus.BAD_REQUEST); | |
| } | |
| } | |
| @Transactional() | |
| async newCreateDevice(user: User, device: DeviceDto) { | |
| this.validateDeviceDto(device); | |
| const newDevice = await this.supportRepository.createDevice(user, { | |
| os: device.os as DeviceOS, | |
| type: device.type as DeviceType, | |
| model: device.model, | |
| }); | |
| await this.redis.del(`user:${user.id}`); | |
| return await this.supportRepository.saveDevice(newDevice); | |
| } |
| const result = await this.essayRepository | ||
| .createQueryBuilder() | ||
| .insert() | ||
| .into('essay') | ||
| .values({ | ||
| title: data.title, | ||
| content: data.content, | ||
| linkedOutGauge: data.linkedOutGauge, | ||
| status: data.status, | ||
| device: data.device, | ||
| author: data.author, | ||
| createdDate: new Date(), | ||
| updatedDate: new Date(), | ||
| coordinates: | ||
| data.longitude && data.latitude | ||
| ? () => `ST_SetSRID(ST_GeomFromText('POINT(${data.longitude} ${data.latitude})'), 4326)` | ||
| : null, | ||
| }) | ||
| .returning('*') | ||
| .execute(); | ||
|
|
||
| const insertedEssayId = result.identifiers[0].id; | ||
|
|
||
| const savedEssay = await this.essayRepository.findOne({ | ||
| where: { id: insertedEssayId }, | ||
| relations: ['tags', 'author', 'device'], | ||
| }); | ||
|
|
||
| if (data.tags && data.tags.length > 0) { | ||
| savedEssay.tags = data.tags; | ||
|
|
||
| await this.essayRepository.save(savedEssay); | ||
| } | ||
|
|
||
| return savedEssay; |
There was a problem hiding this comment.
Add coordinate validation and error handling.
While the PostGIS geometry conversion is implemented correctly, consider adding these safety measures:
- Add coordinate validation:
coordinates:
data.longitude && data.latitude
- ? () => `ST_SetSRID(ST_GeomFromText('POINT(${data.longitude} ${data.latitude})'), 4326)`
+ ? () => {
+ if (data.latitude < -90 || data.latitude > 90 || data.longitude < -180 || data.longitude > 180) {
+ throw new Error('Invalid coordinates: Latitude must be between -90 and 90, Longitude between -180 and 180');
+ }
+ return `ST_SetSRID(ST_GeomFromText('POINT(${data.longitude} ${data.latitude})'), 4326)`;
+ }
: null,- Add error handling for PostGIS operations:
try {
const result = await this.essayRepository
.createQueryBuilder()
// ... existing query ...
.execute();
} catch (error) {
if (error.message.includes('invalid GIS data')) {
throw new Error('Failed to save coordinates: Invalid GIS data format');
}
throw error;
}
|
🎉 This PR is included in version 1.17.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR Type
Summary
바쁜이들을 위한 3줄 요약
Describe your changes
PostGIS 확장 지원 추가:
PostGIS 확장을 사용하여 Essay 엔티티의 좌표를 geometry 타입으로 변경했습니다.
ST_SetSRID와 ST_MakePoint를 활용하여 좌표 데이터를 효율적으로 저장하도록 수정했습니다.
좌표 저장 방식 변경:
좌표 데이터는 이제 geometry(Point, 4326) 타입으로 저장됩니다. 기존 latitude와 longitude 필드는 제거되었으며, coordinates 필드가 이를 대체합니다.
좌표 저장 시 WKT 형식(POINT(longitude latitude))을 사용하며, PostGIS에서 이를 WKB 형식으로 변환하여 저장합니다.
단, 요청의 경우 기존 latitude와 longitude 필드를 그대로 사용합니다
마이그레이션 파일 업데이트:
PostgreSQL에서 PostGIS 확장을 설치하는 마이그레이션을 추가했습니다.
기존 좌표(latitude, longitude) 데이터를 새 coordinates 필드로 이전하고, 불필요해진 필드를 삭제하는 마이그레이션을 작성했습니다.
에세이 조회 시 좌표 필드 변경:
API를 통해 Essay 데이터를 조회할 때 반환되던 기존 latitude와 longitude 필드는 더 이상 사용되지 않으므로 주석 처리했습니다.
대신, 좌표 데이터는 coordinates 필드로 통합되었습니다. 이 필드는 현재 WKB 형식으로 반환되며, 필요에 따라 클라이언트에서 WKT 형식으로 변환해야 합니다.
Issue Number or Link
#456
Summary by CodeRabbit
Release Notes
New Features
BURIEDfor essays, impacting review and retrieval processes.CreateEssayReqDtoto includeBURYin the status options.Bug Fixes
saveEssaymethod to require coordinates when the status isBURIED.Documentation
README.mdfor environment setup and Docker commands.Chores