Skip to content
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

내 장바구니에는 농담곰 100개를 넣을거야 #8

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

hanueleee
Copy link

@hanueleee hanueleee commented May 5, 2023

궁금한 점이 있어서 코멘트 남겨둘게요.. 다들 의견 부탁 ^^

hanueleee and others added 26 commits April 29, 2023 18:17
* docs: 기능 요구사항 정의

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: index.html 파일 수정

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 목록 페이지 연동 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: DB 연결 및 테이블 설계

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 등록 API 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 조회 API 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 수정 API 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: admin.html 파일과 상품 조회 API 연동 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 삭제 API 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 생성 API 연동 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 수정 API 연동 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 상품 삭제 API 연동 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* refactor: Controller GetMapping return type 변경

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 예외 처리 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* refactor: 컨트롤러 책임 분리

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* refactor: 예외 처리 기능 책임 분리

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* refactor: API Url 수정

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* feat: 수정, 삭제 기능에 대한 예외 처리 기능 구현

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* test: ProductController 테스트 추가

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

* refactor: 상품 수정 API의 메소드를 Post에서 Put으로 벼경

* fix: 테스트 에러 수정

* refactor: 상품 생성 및 상품 삭제 API의 상태 코드 변경

* refactor: @controller@RestController로 수정

* refactor: 각 api 별 dto 분리

* refactor: 상품 저장, 수정 테스트 보완

* refactor: ControllerAdvice에 로그 추가

* refactor: service와 repository 계층 간 이동에 dto 대신 entity 사용

* refactor: product 테이블의 imgUrl 컬럼의 자료형을 VARCHAR에서 TEXT로 수정

---------

Co-authored-by: ezzanzzan <eunzzann@gmail.com>
public LoginInterceptor(MemberDao memberDao, AuthorizationExtractor extractor) {
this.memberDao = memberDao;
this.authorizationExtractor = extractor;
}
Copy link
Author

@hanueleee hanueleee May 6, 2023

Choose a reason for hiding this comment

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

현재 AuthorizationExtractor는 인터페이스고 BasicAuthorizationExtractor가 그걸 구현하고 있다.
나중에 확장하면 SessionAuthorizationExtractor이런게 더 생기겠지?

근데 들어오는 인증방식에 따라서 알아서 필요한 extractor를 찾아서 실행시켰으면 좋겠어.
방법1 : 스프링 mvc에서 handler mapping인지 handler adapter인지 구현할 때 map으로 두고 다룰 수 있는애 찾을때까지 순회하자나. 그런식으로 머 canHandle이런애 오버라이딩해놓고 돌게 해야할까
방법2: 아니면 머 bean으로 등록할 때 우선순위같은거 따로 해놓고 할까

Copy link

@woo-chang woo-chang May 6, 2023

Choose a reason for hiding this comment

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

들어오는 인증방식에 따라 필요한 extractor를 실행시키기 위해서는 결국 방법1처럼 모든 extractor를 등록해두고 인증 방식에 알맞은 extractor를 찾는 과정이 필요하지 않을까요? 🤔

순회 시 많이 사용되는 방식을 먼저 조회되도록 해서 오버헤드를 조금이나마 줄일 수 있을 것 같긴합니다!

Copy link

@shin-mallang shin-mallang May 6, 2023

Choose a reason for hiding this comment

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

나였으면 방법1

스프링 비스무리하게 컴포지트 패턴 써가면서 해도 괜찮을 것 같구

근데 여러 인증방식을 쓸 일이 있을라나?

Basic이면 Basic, jwt면 jwt, 세션이면 세션을 쓰지 이들을 혼용해서 쓰지는 않을 것 같은데?

Copy link
Member

Choose a reason for hiding this comment

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

스프링 시큐리티의 경우 아래의 방법을 사용하고 있다고 해

Spring security는 AuthenticationManage 가지고 있는 provider(인증을 수행하는 친구라고 생각) 목록을 순회하면서 provider가 실행 가능한 경우에 provider의 authenticate 메소드를 호출하여 인증 절차를 수행한다.

아이디와 비밀번호, OAuth를 제공하는 사이트가 있다고 가정하고, OAuth도 구글 카카오 여러개가 있다면 각 인증 방법에 해당하는 걸 찾을 때 까지 순회를 하지 않을까 싶네~

image

참고자료

https://gregor77.github.io/2021/05/18/spring-security-03/
https://spring.io/guides/topicals/spring-security-architecture/

Copy link
Member

Choose a reason for hiding this comment

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

허브신

Copy link
Member

@drunkenhw drunkenhw left a comment

Choose a reason for hiding this comment

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

오잉 천재냐 맨날 뛰어댕기는데 왜 잘함

Comment on lines 28 to 42
@GetMapping()
public ResponseEntity<List<ProductDto>> findAllProductInCart(@AuthMemberId int memberId) {
List<ProductDto> allProduct = cartService.findAllProduct(memberId);
return ResponseEntity.status(HttpStatus.OK).body(allProduct);
}

@PostMapping()
public ResponseEntity<Object> addProductToCart(@AuthMemberId int memberId,
@RequestBody CartProductAddRequestDto cartProductAddRequestDto) {
cartService.addProduct(memberId, cartProductAddRequestDto.getProductId());
return ResponseEntity.status(HttpStatus.CREATED).build();
}

@DeleteMapping()
public ResponseEntity<Object> removeProductFromCart(@AuthMemberId int memberId,
Copy link
Member

Choose a reason for hiding this comment

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

어노테이션 괄호 안에 아무것도 없는거 불편해요~ 지워주면 안될까요?
그리고 ResponseEntity가 Object 보단 좀 더 좋은 걸 반환하는게 어떤가요?
아무것도 반환 하지 않는 경우에는 ResponseEntity<Void>를 쓰더라고요

Comment on lines +23 to +30
public List<ProductEntity> findAllByMemberId(int memberId) {
String sql = "SELECT p.id, p.name, p.imgUrl, p.price\n"
+ "FROM cart AS c\n"
+ "INNER JOIN product AS p\n"
+ "ON c.product_id = p.id\n"
+ "WHERE member_id = ?;";
BeanPropertyRowMapper<ProductEntity> mapper = BeanPropertyRowMapper.newInstance(ProductEntity.class);
return jdbcTemplate.query(sql, mapper, memberId);
Copy link
Member

Choose a reason for hiding this comment

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

CartDao에서 findAllByMemberId 를 호출하면 반환 타입이 왠지 CartEntity일거 같아보이는데용

Comment on lines +29 to +33
try {
return Optional.ofNullable(jdbcTemplate.queryForObject(sql, mapper, email, password));
} catch (final EmptyResultDataAccessException e) {
return Optional.empty();
}
Copy link
Member

Choose a reason for hiding this comment

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

메서드로 분리하면 이쁠거 같아용

private static final String BASIC_TYPE = "Basic";
private static final String DELIMITER = ":";

String AUTHORIZATION = "Authorization";
Copy link
Member

Choose a reason for hiding this comment

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

얘는 private static final 해도 괜찮지 않을까용

Copy link
Member

Choose a reason for hiding this comment

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

Spring의 HttpHeaders.AUTHORIZATION을 사용하는건 어떨까

Comment on lines +33 to +42
productDao.findById(id).orElseThrow(() -> new IllegalArgumentException("해당하는 ID가 없습니다."));
productDao.update(new ProductEntity(
id,
productModifyRequestDto.getName(),
productModifyRequestDto.getImgUrl(),
productModifyRequestDto.getPrice()));
}

public void delete(int id) {
productDao.findById(id).orElseThrow(() -> new IllegalArgumentException("해당하는 ID가 없습니다."));
Copy link
Member

Choose a reason for hiding this comment

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

없을 때 예외가 IllegalArgument가 괜찮을까용 더 좋은거 없을까용 귀찮은걸까용

@Override
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer,
NativeWebRequest webRequest, WebDataBinderFactory binderFactory) throws Exception {
AuthInfo authInfo = authorizationExtractor.extract((HttpServletRequest) webRequest.getNativeRequest());
Copy link
Member

Choose a reason for hiding this comment

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

(HttpServletRequest) webRequest.getNativeRequest() 이건 왜 필요하죠

public LoginInterceptor(MemberDao memberDao, AuthorizationExtractor extractor) {
this.memberDao = memberDao;
this.authorizationExtractor = extractor;
}
Copy link
Member

Choose a reason for hiding this comment

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

허브신


Optional<MemberEntity> member = memberDao.findByEmailAndPassword(email, password);
if (member.isPresent()) {
request.setAttribute("memberId", member.get().getId());
Copy link
Member

Choose a reason for hiding this comment

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

요거 쓰이고 있나요? 어디에 쓰이죵?


@DisplayName("상품 저장")
@Nested
class 상품_저장_테스트 {
Copy link
Member

Choose a reason for hiding this comment

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

와 개멋지다

Copy link

@woo-chang woo-chang left a comment

Choose a reason for hiding this comment

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

오잉신! 오잉신! 🐻‍❄️

public ResponseEntity<Object> addProductToCart(@AuthMemberId int memberId,
@RequestBody CartProductAddRequestDto cartProductAddRequestDto) {
cartService.addProduct(memberId, cartProductAddRequestDto.getProductId());
return ResponseEntity.status(HttpStatus.CREATED).build();

Choose a reason for hiding this comment

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

CREATED를 사용할 때는 Location 헤더에 자원에 대한 정보를 주는건 어때?

@ControllerAdvice
public class CartControllerAdvice {

public static final String SERVER_ERROR = "알 수 없는 에러가 발생했습니다.";

Choose a reason for hiding this comment

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

public으로 열려있는 이유가 무엇이죠?!

this.productService = productService;
}

@PostMapping()

Choose a reason for hiding this comment

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

빈 괄호 불편해용~

this.namedParameterJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
}

public int save(ProductEntity productEntity) {

Choose a reason for hiding this comment

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

CartDao에서 save는 void였는데 여기는 int여서 통일시키는것에 대해서는 어떻게 생각하시나요?


@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.hasParameterAnnotation(AuthMemberId.class);

Choose a reason for hiding this comment

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

어노테이션으로만 검증하면 Integer 타입이 아닌 곳에 AuthMemberId 어노테이션이 붙어있어도 resolver가 동작하지 않을까?

Comment on lines +9 to +21
public class WebConfig implements WebMvcConfigurer {

private final LoginArgumentResolver loginArgumentResolver;

public WebConfig(LoginArgumentResolver loginArgumentResolver) {
this.loginArgumentResolver = loginArgumentResolver;
}

@Override
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
resolvers.add(loginArgumentResolver);
}
}

Choose a reason for hiding this comment

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

Interceptor 구현한 것 같은데 인터셉터 등록이 빠진 것 같습니다 .. !

Comment on lines +51 to +59
RestAssured
.given()
.log().all()
.contentType(MediaType.APPLICATION_JSON_VALUE)
.body(productAddRequestDto)
.when()
.post("/products")
.then()
.log().all()

Choose a reason for hiding this comment

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

중복되는 부분은 추출해서 사용하는건 어떨까요? :)

Copy link
Member

@greeng00se greeng00se left a comment

Choose a reason for hiding this comment

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

장바구니에 불이야~ 불이야~

Comment on lines +3 to +41
## 기능 목록
### step1
- [x] 상품 목록 페이지 연동
- [x] index.html 파일 내 TODO 주석을 참고하여 설계한 상품 정보에 맞게 코드를 변경
- [x] index.html 파일을 이용하여 상품 목록이 노출되는 페이지를 완성
- [x] '/' url로 접근할 경우 상품 목록 페이지를 조회
- [x] 상품 기본 정보 : 상품 ID, 상품 이름, 상품 이미지, 상품 가격

- [x] 상품 관리 CRUD API 작성
- [x] Create : POST - /admin
- [x] Read : GET - /admin
- [x] Update : POST - /admin/{id}
- [x] Delete : DELETE - /admin/{id}

- [x] 관리자 도구 페이지 연동
- [x] admin.html, admin.js 파일 내 TODO 주석을 참고하여 코드를 변경
- [x] admin.html 파일과 상품 관리 CRUD API를 이용하여 상품 관리 페이지를 완성
- [x] '/admin' url로 접근할 경우 관리자 도구 페이지를 조회

### step2
- [x] 사용자 기능 구현
- [x] 사용자가 가지고 있는 정보 : email, password

- [x] 사용자 설정 페이지 연동
- [x] settings.html, settings.js 파일 내 TODO 주석을 참고하여 설계한 사용자 정보에 맞게 코드를 변경
- [x] settings.html 파일을 이용해서 사용자를 선택하는 기능을 구현
- [x] /settings url로 접근할 경우 모든 사용자의 정보를 확인하고 사용자를 선택 가능

- [x] 인증 관련
- [x] 사용자 설정 페이지에서 사용자를 선택하면, 이후 요청에 선택한 사용자의 인증 정보가 포함
- [x] 사용자 정보는 요청 Header의 Authorization 필드를 사용해 인증 처리를 하여 획득
- [x] 인증 방식은 Basic 인증 사용

- [x] 장바구니 기능 구현
- [x] 장바구니에 상품 추가
- [x] 장바구니에 담긴 상품 제거
- [x] 장바구니 목록 조회

- [x] 장바구니 페이지 연동
Copy link
Member

Choose a reason for hiding this comment

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

기능 목록 곰곰 👍

Comment on lines +18 to +35
@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<String> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) {
String errorMessage = e.getBindingResult().getAllErrors().get(0).getDefaultMessage();
log.info(errorMessage);
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(errorMessage);
}

@ExceptionHandler(HttpRequestMethodNotSupportedException.class)
public ResponseEntity<String> handleHttpRequestMethodNotSupportedException(HttpRequestMethodNotSupportedException e) {
log.info(e.getMessage());
return ResponseEntity.status(HttpStatus.METHOD_NOT_ALLOWED).body(e.getMessage());
}

@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException e) {
log.info(e.getMessage());
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(e.getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

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

예외의 대한 로깅은 info보다 warn으로 조정해보는건 어떨까?

Comment on lines 18 to 35
@RestController
@RequestMapping("/products")
public class ProductController {

private final ProductService productService;

public ProductController(ProductService productService) {
this.productService = productService;
}

@PostMapping()
public ResponseEntity<Integer> productAdd(@Validated @RequestBody ProductAddRequestDto productAddRequestDto) {
int productId = productService.save(productAddRequestDto);
return ResponseEntity.status(HttpStatus.CREATED).location(URI.create("/products/" + productId)).build();
}

@PutMapping("/{id}")
public ResponseEntity<Object> productModify(@Validated @RequestBody ProductModifyRequestDto productModifyRequestDto,
Copy link
Member

Choose a reason for hiding this comment

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

표준 스펙인 Valid 대신 Spring의 Validated를 사용한 이유가 있을까?

}

public List<ProductEntity> findAllByMemberId(int memberId) {
String sql = "SELECT p.id, p.name, p.imgUrl, p.price\n"
Copy link
Member

Choose a reason for hiding this comment

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

개행 대신 빈칸은 어떨까?

private static final String BASIC_TYPE = "Basic";
private static final String DELIMITER = ":";

String AUTHORIZATION = "Authorization";
Copy link
Member

Choose a reason for hiding this comment

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

Spring의 HttpHeaders.AUTHORIZATION을 사용하는건 어떨까

Copy link
Member

@greeng00se greeng00se left a comment

Choose a reason for hiding this comment

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

장바구니에 불이야~ 불이야~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants