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

제버릇 개 못주고 깔끔하게 구현하지 못한 누누 #9

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

Conversation

be-student
Copy link

No description provided.

be-student and others added 30 commits May 2, 2023 19:58
* docs: 기능목록 작성

* feat: get /products Controller 구현

* feat: put /products/{id} Controller 구현

* feat: post /products Controller 구현

* feat: delete /products/{id} Controller 구현

* feat: get / Controller 구현

* feat: get /admin Controller 구현

* feat: CreateProductService 구현

* feat: ProductName 구현

* feat: ProductImage 구현

* feat: ProductPrice 구현

* feat: ProductId 구현

* feat: Product 구현

* feat: ProductDao 구현

* feat: H2ProductRepository 구현

* feat: CreateProductService 구현

* feat: UpdateProductService 구현

* refactor: ProductService 네이밍 변경

* feat: ProductSearchService 구현

* feat: ProductDeleteService 구현

* feat: ProductSearchController 구현

* feat: ProductUpdateController 구현

* feat: ProductCreateController 구현

* feat: ProductDeleteController 구현

* fix: TemplateControllerTest 수정

* docs: 기능목록 업데이트

* feat: ProductViewController 구현

* fix: ProductCreateController 에서 서비스를 호출하도록 수정

* feat: yml 파일 생성

* fix: ProductViewControllerTest mockBean 추가

* feat: admin 정적 리소스 작성

* feat: index.html 작성

* docs: readme 최신화

* feat: exceptionHandler 작성

* feat: valid 로 dto 검증 기능 추가

* refactor: 개행, suppress warnings 작업

* chore: reformat code

* feat: logger warn 으로 변경

* refactor: service 분리되어 있던 부분 합침

* refactor: controller 분리되어 있던 부분 합침

* chore: dao 제거

* refactor: productId 제거

* refactor: productName 제거

* refactor: getter 로 직접 검증하지 않고, usingRecursiveComparison 을 사용

* refactor: test 클래스 분리되어있던 부분 합침

* chore: 안 쓰이는 코드 제거

---------

Co-authored-by: odo27 <[email protected]>
@be-student be-student self-assigned this May 5, 2023
Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

보기 좋있습니다 누누

@@ -6,8 +6,7 @@
@SpringBootApplication
public class JwpCartApplication {

public static void main(String[] args) {
public static void main(final String[] args) {

Choose a reason for hiding this comment

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

이걸 일케까지??
자동설정인가??

Copy link
Author

Choose a reason for hiding this comment

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

어 자동 설정

Choose a reason for hiding this comment

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

final 붙인거갖고 그러는거야ㅕ?

import org.apache.tomcat.util.codec.binary.Base64;
import org.springframework.web.filter.OncePerRequestFilter;

public class AuthenticationFilter extends OncePerRequestFilter {

Choose a reason for hiding this comment

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

왜 인터셉터 대신 filter를 쓰셨죵?
OncePerRequestFilter는 역할이 뭐고 왜 이걸 상속하셨나요???
갓누누

Copy link
Author

Choose a reason for hiding this comment

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

interceptor 는 기본적으로 dispatcherServlet 을 거쳤다 가기에, spring web 에 종속적인 구조가 됩니다
filter 의 경우에는, 한번 잘 구조를 잡아두면 여러 환경에서 사용할 수 있다는 점이 인증에 효과적이라서 사용하게 되었습니다

Comment on lines 25 to 37
public Object resolveArgument(final MethodParameter parameter, final ModelAndViewContainer mavContainer,
final NativeWebRequest webRequest, final WebDataBinderFactory binderFactory) {
final String header = webRequest.getHeader(AUTHORIZATION);

if (header == null || !header.toLowerCase().startsWith(BASIC_TYPE.toLowerCase())) {
throw new IllegalArgumentException();
}

final String authHeaderValue = header.substring(BASIC_TYPE.length()).trim();

final String[] credentials = new String(decodeBase64(authHeaderValue)).split(":");
return new AuthInfo(credentials[EMAIL_INDEX]);
}

Choose a reason for hiding this comment

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

filter에서 해준 행위를 거의 중복으로 해주는 것 같은데??

Copy link
Author

Choose a reason for hiding this comment

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

그치? 거의 대부분 중복이긴 한데, 이정도는 괜찮아 보여서 그냥 했어

Choose a reason for hiding this comment

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

중복 22


public class CartProduct {

private final CartProductId cartProductId;

Choose a reason for hiding this comment

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

그냥 id로 했을때와 이렇게 했을때 비교해서 장점이 뭔가요 누누

Copy link
Author

Choose a reason for hiding this comment

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

다른 id랑 import 했을 때 안 헷갈린다?

private static final int MAX_NAME_LENGTH = 255;

private final ProductId productId;
private final String productName;

Choose a reason for hiding this comment

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

왜 Name은 객체로 안만들었나요??

Copy link
Author

Choose a reason for hiding this comment

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

저때 이름은 어차피 검증 거의 없을 거고 변하지 않을거라는 내 나름의 해석을 하고 그냥 없애버렸어 귀찮아

Choose a reason for hiding this comment

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

다른건 다했는데 서운함


private final CartDao cartDao;
private final CartProductDao cartProductDao;
private final ProductRepository productRepository;

Choose a reason for hiding this comment

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

repository가 repository를 가진 구조라

Copy link
Author

Choose a reason for hiding this comment

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

같은 레이어간 참조를 없애려면 직접 dao 를 참조해야하는데, 그렇게까지...?
그냥 repo 참조해버렸어 귀찮았거든

this.productQueryService = productQueryService;
}

@EventListener(UserRegisteredEvent.class)

Choose a reason for hiding this comment

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

handler는 service에 같이 묶는 것 보다는
CreateCartWithUserRegisteredEventHandler 식으로 네이밍 짓는게 일반적인 것 같은 것 같은 생각이 드는 것 같은 느낌적인 느낌

Copy link
Author

Choose a reason for hiding this comment

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

비즈니스 로직이니까 그렇게도 볼 수 있는거 아닐까?

네이밍은 고민해봐야 하는게 맞긴 한 것 같아
카트에 대한 변경이 있다는 점만 생각하면 이 클래스에 있어도 괜찮아보이는데 어때?


@Service
@Transactional(readOnly = true)
public class CartQueryService {

Choose a reason for hiding this comment

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

command와 query 단위로 분리하셨는데
장점이 뭐였을까요

Copy link
Author

Choose a reason for hiding this comment

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

추후 CQRS?


public class UserRegisteredEvent {

private final User user;

Choose a reason for hiding this comment

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

이벤트에는 id만 넣는 식으로 하는것도 갠차낭

Copy link
Author

Choose a reason for hiding this comment

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

오 이건 처음 알았네
굿굿


public User save(final String email, final String password) {
final User user = userRepository.save(new User(email, password));
applicationEventPublisher.publishEvent(new UserRegisteredEvent(user));

Choose a reason for hiding this comment

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

오호 이벤트 활용을 잘 하셨네요
혹시 어떤 자료로 공부했는지 알 수 있을까요 ^^

Copy link
Author

Choose a reason for hiding this comment

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

신의 자료로 공부했습니다

Copy link

@Choi-JJunho Choi-JJunho 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 25 to 37
public Object resolveArgument(final MethodParameter parameter, final ModelAndViewContainer mavContainer,
final NativeWebRequest webRequest, final WebDataBinderFactory binderFactory) {
final String header = webRequest.getHeader(AUTHORIZATION);

if (header == null || !header.toLowerCase().startsWith(BASIC_TYPE.toLowerCase())) {
throw new IllegalArgumentException();
}

final String authHeaderValue = header.substring(BASIC_TYPE.length()).trim();

final String[] credentials = new String(decodeBase64(authHeaderValue)).split(":");
return new AuthInfo(credentials[EMAIL_INDEX]);
}

Choose a reason for hiding this comment

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

중복 22

import cart.domain.cart.CartId;
import java.util.Objects;

public class CartEntityId {

Choose a reason for hiding this comment

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

ㅇㅈ

Comment on lines +36 to +37


Choose a reason for hiding this comment

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


try {
jdbcTemplate.update(sql, id);
} catch (final DataIntegrityViolationException e) {
throw new ProductInCartDeleteException("상품이 장바구니에 존재합니다. 장바구니에서 먼저 삭제해주세요.");

Choose a reason for hiding this comment

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

오 좋은데요?

Choose a reason for hiding this comment

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

CASCADE 조지기 어때?


@Override
public boolean existsByEmailAndPassword(final String email, final String password) {
final String sql = "SELECT EXISTS(SELECT 1 FROM MEMBER WHERE email=? AND password=?)";

Choose a reason for hiding this comment

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

EXISTS 👍

Copy link

@gitchannn gitchannn left a comment

Choose a reason for hiding this comment

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

깃짱 다녀감

@@ -1 +1,50 @@
# jwp-shopping-cart

Choose a reason for hiding this comment

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

File changed 104 good

@@ -6,8 +6,7 @@
@SpringBootApplication
public class JwpCartApplication {

public static void main(String[] args) {
public static void main(final String[] args) {

Choose a reason for hiding this comment

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

final 붙인거갖고 그러는거야ㅕ?

import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;

@Configuration
public class SecurityConfig implements WebMvcConfigurer {

Choose a reason for hiding this comment

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

옼 이름 좀 간지 GDD

- [x] delete product
- [x] 관리자 도구 페이지 연동
- [x] admin.html 변경
- [x] admin.js 변경

Choose a reason for hiding this comment

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

API 설계한거 URL도 같이 적어주면 (내가) 매우 좋을 듯

@RestControllerAdvice
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {

private final Logger log = LoggerFactory.getLogger(getClass());

Choose a reason for hiding this comment

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

이렇게 로그 찍으면 어디서 어떻게 확인할 수 있어??

() -> assertThat(result.body().jsonPath().getLong("productId")).isEqualTo(1),
() -> assertThat(result.body().jsonPath().getString("name")).isEqualTo("누누"),
() -> assertThat(result.body().jsonPath().getString("image")).isEqualTo("naver.com"),
() -> assertThat(result.body().jsonPath().getInt("price")).isEqualTo(1)

Choose a reason for hiding this comment

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

그냥 .body()로 테스트 안하고 왜 분리한거야??

}

@Test
void 인증_헤더_있이_요청시_성공한다() throws Exception {

Choose a reason for hiding this comment

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

없이 있이...? 에반데

final String result = objectMapper.writeValueAsString(NUNU_RESPONSE);

// when
mockMvc.perform(post("/products")

Choose a reason for hiding this comment

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

mockMvc..? 다음에 물어보러 올게
초심자를 위한 포스팅 해주는거 어땡

import org.springframework.http.MediaType;

@SuppressWarnings({"NonAsciiCharacters"})
class ViewControllerTest extends AbstractControllerTest {

Choose a reason for hiding this comment

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

근데 ControllerTest가 IntegrationTest 아니야??

TRUNCATE TABLE PRODUCT;
TRUNCATE TABLE MEMBER;
SET
FOREIGN_KEY_CHECKS = 1;

Choose a reason for hiding this comment

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

이게 뭐하는거야?

해당 SQL은 데이터베이스 테이블의 데이터를 모두 삭제하는 것으로 보입니다.

데이터를 삭제하기 전에 FOREIGN_KEY_CHECKS 값을 0으로 설정하고, 삭제 후에는 1로 설정하는 것으로 보아, 이 SQL은 외래 키 제약 조건이 있는 테이블에서 데이터를 삭제할 때 사용됩니다.

외래 키 제약 조건이 있는 테이블에서 데이터를 삭제할 때는 먼저 해당 테이블의 외래 키 제약 조건을 해제해야 합니다. 그래야만 데이터를 삭제할 수 있습니다. 이 SQL은 그 과정을 자동화한 것으로 보입니다.

하지만 이 SQL이 완전히 안전한 것은 아닙니다. 외래 키 제약 조건이 있는 테이블에서 데이터를 삭제하는 것은 매우 위험한 작업입니다. 데이터를 삭제하기 전에 꼭 백업을 하고, 삭제 후에는 외래 키 제약 조건을 다시 설정해야 합니다.

이런 거 마즘??

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.

4 participants