-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 검수할 장소 등록/ 조회 API 구현 #404
Changes from 17 commits
5833293
9b4f610
b446c17
0c13a93
7a757de
b9693dc
29d6004
efbfd65
190a739
85afada
a80562e
3cd6562
96a2a93
4fa26c4
421daa0
01ebc2c
884e107
890d080
2df2100
0cdae62
0b65425
c70ae34
3057dba
2be9c53
647be5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ public enum PlaceExceptionType implements BaseExceptionType { | |
HttpStatus.BAD_REQUEST, | ||
"이미 주변에 장소가 존재합니다." | ||
), | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: |
||
; | ||
|
||
private final int errorCode; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
package com.now.naaga.temporaryplace.application; | ||
|
||
import com.now.naaga.common.infrastructure.FileManager; | ||
import com.now.naaga.place.domain.Position; | ||
import com.now.naaga.player.application.PlayerService; | ||
import com.now.naaga.player.domain.Player; | ||
import com.now.naaga.temporaryplace.application.dto.CreateTemporaryPlaceCommand; | ||
import com.now.naaga.temporaryplace.domain.TemporaryPlace; | ||
import com.now.naaga.temporaryplace.repository.TemporaryPlaceRepository; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
import java.io.File; | ||
import java.util.Comparator; | ||
import java.util.List; | ||
|
||
@Transactional | ||
@Service | ||
public class TemporaryPlaceService { | ||
|
||
private final TemporaryPlaceRepository temporaryPlaceRepository; | ||
|
||
private final PlayerService playerService; | ||
|
||
private final FileManager<MultipartFile> fileManager; | ||
|
||
public TemporaryPlaceService(final TemporaryPlaceRepository temporaryPlaceRepository, | ||
final PlayerService playerService, | ||
final FileManager<MultipartFile> fileManager) { | ||
this.temporaryPlaceRepository = temporaryPlaceRepository; | ||
this.playerService = playerService; | ||
this.fileManager = fileManager; | ||
} | ||
|
||
public TemporaryPlace createTemporaryPlace(final CreateTemporaryPlaceCommand createTemporaryPlaceCommand) { | ||
final Position position = createTemporaryPlaceCommand.position(); | ||
final File uploadPath = fileManager.save(createTemporaryPlaceCommand.imageFile()); | ||
try { | ||
final Long playerId = createTemporaryPlaceCommand.playerId(); | ||
final Player registeredPlayer = playerService.findPlayerById(playerId); | ||
final TemporaryPlace temporaryPlace = new TemporaryPlace( | ||
createTemporaryPlaceCommand.name(), | ||
createTemporaryPlaceCommand.description(), | ||
position, | ||
fileManager.convertToUrlPath(uploadPath), | ||
registeredPlayer); | ||
return temporaryPlaceRepository.save(temporaryPlace); | ||
} catch (final RuntimeException exception) { | ||
uploadPath.delete(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P5: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 따로 빼서 이에 대한 로직을 캡슐화할 수 있을 것 같아요. 저도 추상적으로만 생각되긴 하는데, 배포 이후에 개선 방안을 생각해보면 좋을 것 같긴 하네요!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋습니다 해당 내용 이슈로 파두도록 하겠습니다 |
||
throw exception; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: 저희 아직 플레이어 장소 등록에 20미터 검증에 대한 명확한 기획 변경이 확정되지 않았습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 아직 변경되지 않은 기획에 대해서는, 기존 기획 그대로 가는 것에 동의하는데요! 생각해보니 이번 배포 주기에 어떻게 하면 좋을까요? 뷰가 따로 구현되어있지 않다면 아마 사용자는 앱에서 장소 등록을 해도 무반응이 올 것 같기도...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이부분에 대해서는 위에 리뷰 포인트로 남겨두었는데요. 슬랙에서 얘기 나눈대로 안드 쪽에서도 아직 구현이 안된 부분이라 후일에 논의가 끝나면 다시 추가해도 될 것 같습니다 |
||
|
||
@Transactional(readOnly = true) | ||
public List<TemporaryPlace> findAllPlace() { | ||
final List<TemporaryPlace> temporaryPlaces = temporaryPlaceRepository.findAll(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: @Entity
public class TemporaryPlace extends BaseEntity {
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Id
private Long id;
// ...
@ManyToOne
@JoinColumn(name = "player_id")
private Player registeredPlayer;
// ...
} 기본적으로 현재 뷰로 넘어가는 응답을 보면 public record TemporaryPlaceResponse(Long id,
String name,
CoordinateResponse coordinate,
String imageUrl,
String description) {
// ...
} 등록자에 대한 정보는 안거드리므로 join은 필요가 없을 것 같아요! 그래서 제 생각에는 지연로딩으로 1차적으로 N+1 쿼리가 발생하지 않도록 하는 것이어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: 생각해보니 현재 전체 조회 API 명세에 문제가 있네요! 검수 장소에서 실제 인게임 장소로 등록되기 위해선( 때문에 또한 위에서 루카가 잘 말씀해주셨는데, API 명세에서 playerId 가 추가되므로 이제 player 객체의 조회가 필요해져서 N+1 문제가 발생할 것 같습니다. 일단 연관관계에 대한 로딩을 지연로딩으로 바꾸고, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API명세에 player관련 명세가 없어서 아무생각없이 코드를 짜버렸네요. 조회 쿼리를 다루면서 N+1문제에 대해서도 깊은 고민을 안했군요...ㅎㅎ |
||
temporaryPlaces.sort(Comparator.comparing(TemporaryPlace::getCreatedAt)); | ||
return temporaryPlaces; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||||
package com.now.naaga.temporaryplace.application.dto; | ||||||
|
||||||
import com.now.naaga.place.domain.Position; | ||||||
import com.now.naaga.player.presentation.dto.PlayerRequest; | ||||||
import com.now.naaga.temporaryplace.presentation.dto.CreateTemporaryPlaceRequest; | ||||||
import org.springframework.web.multipart.MultipartFile; | ||||||
|
||||||
public record CreateTemporaryPlaceCommand(Long playerId, | ||||||
String name, | ||||||
String description, | ||||||
Position position, | ||||||
MultipartFile imageFile) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2:
Suggested change
띄어쓰기 2번 입니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 고쳤습니다! |
||||||
|
||||||
public static CreateTemporaryPlaceCommand of(final PlayerRequest playerRequest, | ||||||
final CreateTemporaryPlaceRequest createTemporaryPlaceRequest) { | ||||||
Position position = Position.of(createTemporaryPlaceRequest.latitude(), createTemporaryPlaceRequest.longitude()); | ||||||
return new CreateTemporaryPlaceCommand( | ||||||
playerRequest.playerId(), | ||||||
createTemporaryPlaceRequest.name(), | ||||||
createTemporaryPlaceRequest.description(), | ||||||
position, | ||||||
createTemporaryPlaceRequest.imageFile()); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,47 @@ | ||
package com.now.naaga.temporaryplace.presentation; | ||
|
||
import org.springframework.web.bind.annotation.RequestMapping; | ||
import org.springframework.web.bind.annotation.RestController; | ||
import com.now.naaga.auth.presentation.annotation.Auth; | ||
import com.now.naaga.player.presentation.dto.PlayerRequest; | ||
import com.now.naaga.temporaryplace.application.TemporaryPlaceService; | ||
import com.now.naaga.temporaryplace.application.dto.CreateTemporaryPlaceCommand; | ||
import com.now.naaga.temporaryplace.domain.TemporaryPlace; | ||
import com.now.naaga.temporaryplace.presentation.dto.CreateTemporaryPlaceRequest; | ||
import com.now.naaga.temporaryplace.presentation.dto.TemporaryPlaceResponse; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.*; | ||
|
||
import java.net.URI; | ||
import java.util.List; | ||
|
||
@RequestMapping("/temporary-places") | ||
@RestController | ||
public class TemporaryPlaceController { | ||
|
||
private final TemporaryPlaceService temporaryPlaceService; | ||
|
||
public TemporaryPlaceController(final TemporaryPlaceService temporaryPlaceService) { | ||
this.temporaryPlaceService = temporaryPlaceService; | ||
} | ||
|
||
@GetMapping | ||
public ResponseEntity<List<TemporaryPlaceResponse>> findAllTemporaryPlace() { | ||
final List<TemporaryPlace> temporaryPlaces = temporaryPlaceService.findAllPlace(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: 서비스의 메서드명 일관성이 깨지는 것 같아요 ㅠㅠ
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. api 명세 얘기가 될 것 같은데요,,! 저희는 지금까지 정렬이 필요한 모든 api에 쿼리 스트링으로 조건을 달아주었습니다. 서비스 로직에서 시간 순으로 정렬을 하는 것이 보이는데 쿼리스트링으로 조건을 걸어줄 필요성에 대해서 이래 의견은 어떠신가요!? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이것도 얘기를 나눠봐야 할 것 같은데요 API명세 상으로는 아직 관련 내용이 적용되지 않아서.
|
||
final List<TemporaryPlaceResponse> response = TemporaryPlaceResponse.convertToResponses(temporaryPlaces); | ||
return ResponseEntity | ||
.status(HttpStatus.OK) | ||
.body(response); | ||
} | ||
|
||
@PostMapping | ||
public ResponseEntity<TemporaryPlaceResponse> createTemporaryPlace(@Auth final PlayerRequest playerRequest, | ||
@ModelAttribute final CreateTemporaryPlaceRequest createTemporaryPlaceRequest) { | ||
final CreateTemporaryPlaceCommand createTemporaryPlaceCommand = CreateTemporaryPlaceCommand.of(playerRequest, createTemporaryPlaceRequest); | ||
final TemporaryPlace temporaryPlace = temporaryPlaceService.createTemporaryPlace(createTemporaryPlaceCommand); | ||
final TemporaryPlaceResponse response = TemporaryPlaceResponse.from(temporaryPlace); | ||
return ResponseEntity | ||
.status(HttpStatus.CREATED) | ||
.location(URI.create("/temporary-places/" + temporaryPlace.getId())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P5: 근데 현재 이 로케이션에 대한 API는 생성이 안되어 있는데, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 관리자 페이지에서 임시장소 하나하나를 따로 볼 수도 있고 후에 확장성을 고려해서 위와 같이 코드를 짰숩니다~ |
||
.body(response); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.now.naaga.temporaryplace.presentation.dto; | ||
|
||
import org.springframework.web.multipart.MultipartFile; | ||
|
||
public record CreateTemporaryPlaceRequest(String name, | ||
String description, | ||
Double latitude, | ||
Double longitude, | ||
MultipartFile imageFile) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package com.now.naaga.temporaryplace.presentation.dto; | ||
|
||
import com.now.naaga.game.presentation.dto.CoordinateResponse; | ||
import com.now.naaga.temporaryplace.domain.TemporaryPlace; | ||
|
||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public record TemporaryPlaceResponse(Long id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
String name, | ||
CoordinateResponse coordinate, | ||
String imageUrl, | ||
String description | ||
|
||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: 불필요한 개행은 삭제해주세요. |
||
|
||
public static TemporaryPlaceResponse from(final TemporaryPlace savedTemporaryPlace) { | ||
CoordinateResponse coordinateResponse = CoordinateResponse.of(savedTemporaryPlace.getPosition()); | ||
return new TemporaryPlaceResponse( | ||
savedTemporaryPlace.getId(), | ||
savedTemporaryPlace.getName(), | ||
coordinateResponse, | ||
savedTemporaryPlace.getImageUrl(), | ||
savedTemporaryPlace.getDescription() | ||
); | ||
} | ||
|
||
public static List<TemporaryPlaceResponse> convertToResponses(final List<TemporaryPlace> temporaryPlaces) { | ||
return temporaryPlaces.stream() | ||
.map(TemporaryPlaceResponse::from) | ||
.collect(Collectors.toList()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ | |
value="/home/ubuntu/logs"/> | ||
</springProfile> | ||
|
||
<root level="DEBUG"> | ||
<root level="INFO"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P5: 저도 DEBUG 레벨 때문에 많이 불편했었는데 고쳐주셨네요!! 다만, 로깅 레벨을 바꾸는 것은 이 PR의 작업 내용과 관련이 없다고 생각해서, 이는 변경사항에서 빼주는 것도 좋을 것 같아요! 물론 변경점이 매우매우매우 작기 때문에 이정도는 괜찮긴하겠네요 ㅎㅎ |
||
<appender-ref ref="CONSOLE"/> | ||
</root> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 수정이네요👍