refactor: Improve API design with RESTful conventions and Rails crede… #10
Conversation
…ntials API Changes: - Change POST /places/:id/like to POST /places/:place_id/likes for better REST semantics - Split PlacesController to separate concerns (PlacesController + PlaceLikesController) - Allow users to like any place, not just their own places - Remove authentication from external search endpoint for public access Code Quality Improvements: - Migrate from .env to Rails credentials for secure configuration management - Add SQL injection protection to search queries with sanitize_sql_like - Remove obsolete Folder model association from User model - Update Swagger documentation to reflect new API structure Testing: - Create comprehensive RSpec test suite with 24 examples - Add test environment credentials for proper JWT authentication in tests - Fix nullable field schemas in Swagger specs - All tests passing (26 examples, 0 failures) - Rubocop lint checks passing
|
Caution Review failedThe pull request is closed. Walkthrough경로 탐색(방향 찾기) 기능을 완전히 제거하고, 장소 기반 API로 재설계된 변경사항입니다. 기존 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/v1/my_search_controller.rb (1)
69-69: Add index onlikes_countfor ORDER BY performance.The code orders by
likes_countdescending, which requires a database index for efficient query performance as the dataset grows. Add an index onlikes_countor a composite index on(likes_count, created_at)to avoid full table scans.
🧹 Nitpick comments (2)
app/controllers/api/v1/external_controller.rb (1)
6-45: Consider rate limiting for the public search endpoint.Making the external search endpoint public aligns with the PR objectives. However, public APIs are susceptible to abuse and excessive usage. Consider implementing rate limiting (e.g., using
rack-attackgem) to protect against:
- Excessive requests from individual IPs
- Potential cost implications from proxying Naver API calls
- Service degradation from abuse
Example rate limiting configuration
If you add
rack-attackto your Gemfile, you could configure something like:# config/initializers/rack_attack.rb Rack::Attack.throttle("external_search/ip", limit: 30, period: 1.minute) do |req| req.ip if req.path == "/api/v1/external/search" endapp/controllers/api/v1/my_search_controller.rb (1)
83-85: Consider using Rails' built-inActiveRecord::Base.sanitize_sql_like.Rails provides a built-in
sanitize_sql_likemethod inActiveRecord::Basethat handles LIKE pattern escaping. Using the framework's method is preferable for maintainability and consistency.🔎 Proposed refactor
- def sanitize_sql_like(string) - string.gsub(/[\\%_]/) { |m| "\\#{m}" } - end + def sanitize_sql_like(string) + ActiveRecord::Base.sanitize_sql_like(string) + endOr simply call
ActiveRecord::Base.sanitize_sql_like(query)directly at each usage site and remove this helper method entirely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/controllers/api/v1/courses_controller.rb(0 hunks)app/controllers/api/v1/directions_controller.rb(0 hunks)app/controllers/api/v1/external_controller.rb(1 hunks)app/controllers/api/v1/my_search_controller.rb(2 hunks)app/controllers/api/v1/place_likes_controller.rb(1 hunks)app/controllers/api/v1/places_controller.rb(1 hunks)app/models/user.rb(0 hunks)app/services/naver_directions_service.rb(0 hunks)app/services/naver_search_service.rb(3 hunks)app/services/odsay_transit_service.rb(0 hunks)config/credentials/development.yml.enc(1 hunks)config/credentials/test.yml.enc(1 hunks)config/database.yml(1 hunks)config/initializers/omniauth.rb(1 hunks)config/routes.rb(1 hunks)spec/requests/api/v1/courses_spec.rb(0 hunks)spec/requests/api/v1/directions_spec.rb(0 hunks)spec/requests/api/v1/my_search_spec.rb(1 hunks)spec/requests/api/v1/places_spec.rb(1 hunks)spec/requests/api/v1/popular_places_spec.rb(1 hunks)swagger/v1/swagger.yaml(1 hunks)test/controllers/api/v1/directions_controller_test.rb(0 hunks)
💤 Files with no reviewable changes (8)
- app/models/user.rb
- spec/requests/api/v1/courses_spec.rb
- app/controllers/api/v1/courses_controller.rb
- app/services/naver_directions_service.rb
- test/controllers/api/v1/directions_controller_test.rb
- app/services/odsay_transit_service.rb
- app/controllers/api/v1/directions_controller.rb
- spec/requests/api/v1/directions_spec.rb
🧰 Additional context used
🧬 Code graph analysis (3)
app/controllers/api/v1/place_likes_controller.rb (2)
app/controllers/api/v1/places_controller.rb (2)
before_action(3-57)set_place(33-37)app/controllers/application_controller.rb (1)
current_user(36-38)
app/controllers/api/v1/places_controller.rb (1)
app/controllers/api/v1/place_likes_controller.rb (1)
before_action(3-38)
spec/requests/api/v1/places_spec.rb (1)
app/services/json_web_token.rb (1)
encode(4-7)
🔇 Additional comments (17)
app/services/naver_search_service.rb (3)
20-36: LGTM: Consistent credential pattern for local search.The credential access pattern change aligns with the broader migration to Rails credentials. The safe navigation operator properly handles missing credentials.
38-55: Fix API endpoint for Naver Cloud Platform Geocoding API.The endpoint is incorrect: code uses
maps.apigw.ntruss.combut official NCP documentation specifieshttps://naveropenapi.apigw.ntruss.com/map-geocode/v2. This will cause authentication or routing failures. Additionally, verify that credentials changed fromnaver_cloudtonaverare compatible with NCP's Geocoding service, as separate credentials may be required for Naver Cloud Platform Maps versus Naver Developers API.
10-17: Verify intentional API contract change:search_placesno longer attempts geocoding fallback.The method now only returns local search results and returns an empty array on error, removing the fallback to address geocoding that may have existed previously. The separate
search_addressmethod still provides geocoding functionality.Confirm with the team that clients relying on
search_placesfor address-based searches won't be impacted. The public API endpoint inExternalControllerwill now only return local place results, not geocoding results.spec/requests/api/v1/popular_places_spec.rb (1)
1-44: LGTM: Well-structured API specification test.The test properly documents the Popular Places endpoint with:
- Comprehensive response schema including all place fields
- Type definitions and descriptions for each property
- Required fields specification
- Integration with Swagger documentation
app/controllers/api/v1/places_controller.rb (1)
5-5: LGTM: Correct RESTful refactoring of like functionality.The removal of the
likeaction and its correspondingbefore_actionfilter correctly implements the separation of concerns. The like functionality moved toPlaceLikesController(visible in the related snippets), which:
- Uses
Place.find(params[:place_id])instead ofcurrent_user.places.find- Allows users to like any place, not just their own
- Follows RESTful conventions with
POST /api/v1/places/:place_id/likesThe
set_placemethod's restriction tocurrent_user.placesremains appropriate for theshowaction in this controller.config/initializers/omniauth.rb (1)
11-14: Environment-specific credentials are properly supported in Rails 8.1.1.The credential access pattern using safe navigation (
Rails.application.credentials.kakao&.client_id) is correct. Rails 6 added support for multi-environment credentials with separate files and encryption keys, and Rails 8.1.1 fully supports this feature.Verify that credentials files exist with the correct structure:
config/credentials/development.yml.enc(or environment-specific file for current environment)config/credentials/production.yml.enc(for production)- Each file contains a top-level
kakaokey with nestedclient_idandclient_secretThe safe navigation operator usage is appropriate for handling nil-safe access to nested credential keys.
app/controllers/api/v1/my_search_controller.rb (1)
58-58: Good SQL injection protection implementation.The sanitization is correctly applied before string interpolation in LIKE patterns, and the
.present?guards ensure nil values won't reach the sanitization method.Also applies to: 62-67, 77-77
app/controllers/api/v1/place_likes_controller.rb (3)
9-29: LGTM! Clean toggle implementation.The like/unlike toggle logic is clear and correctly handles both adding and removing likes. Using bang methods (
destroy!,create!) ensures failures raise exceptions that will be caught by the default error handler.
33-37: Design change allows liking any place (not just own places).This implementation allows users to like any place in the system, not just their own. This differs from
PlacesController#set_place(which scopes tocurrent_user.places) and aligns with the PR objective: "Allow users to like any place (no longer restricted to their own places)."
17-17: Thecounter_cache: :likes_countconfiguration is correctly set up.The reload calls are necessary to refresh the in-memory object after creating or destroying place likes. Since Rails automatically updates the database via counter_cache, reload ensures the response reflects the updated count.
Also applies to: 25-25
spec/requests/api/v1/places_spec.rb (2)
1-170: Comprehensive test coverage for Places API.The test suite provides excellent coverage of success and error scenarios (200, 401, 404) for all Places API endpoints. The schema definitions are detailed and include proper nullable field annotations.
102-110: Good practice: Explicit nullable field annotations.The schema properly marks optional fields as
nullable: true, which improves API documentation accuracy and helps consumers understand which fields may be absent.spec/requests/api/v1/my_search_spec.rb (1)
1-96: Well-structured test specifications for search endpoints.The tests provide clear documentation of the search API parameters and response schemas. The enum specification for the
typeparameter is particularly helpful for API consumers.config/routes.rb (1)
39-44: Excellent RESTful design for nested likes resource.Using a nested singular resource for likes (
POST /places/:place_id/likes) properly represents the relationship and follows REST conventions. The route structure is intuitive and self-documenting.swagger/v1/swagger.yaml (3)
592-627: Accurate documentation for likes toggle endpoint.The Swagger definition correctly documents the RESTful like toggle endpoint with appropriate parameter naming (
place_id), response schema, and security requirements.
554-587: Consistent nullable field annotations across documentation.The Swagger schema properly marks optional fields as
nullable: true, consistent with the RSpec specifications. This attention to detail improves API documentation quality.
189-270: Verify external search endpoint authentication requirement.The Swagger documentation correctly indicates this endpoint is public (no
securitydeclaration), and this aligns with the implementation. TheApi::V1::ExternalController#searchaction does not declarebefore_action :require_login. The baseApplicationControllerapplies an optionalauthenticate_requestfilter that only attempts JWT verification if an Authorization header is present; unauthenticated requests are allowed.
Remove course-related search capabilities from my_search controller. Update SQL sanitization to use ActiveRecord::Base.sanitize_sql_like. Remove courses_spec file as course functionality has been removed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolve conflicts by accepting improved credentials access pattern and removing course-related functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ntials
API Changes:
Code Quality Improvements:
Testing:
Summary by CodeRabbit
릴리스 노트
새 기능
변경 사항
✏️ Tip: You can customize this high-level summary in your review settings.