Skip to content

Conversation

@devjun10
Copy link
Contributor

@devjun10 devjun10 commented Feb 5, 2024

📝 구현 내용

애플리케이션 내부에 저장하던 사용자 정보를 MySQL 내부에 저장했습니다.

  • 데이터베이스 교체(MySQL)
  • Jdbc 모듈 추가
  • Jdbc 템플릿 구현
  • 트랜잭션 구현






✨ JdbcTemplate

JdbcTemplate을 Template Callback 패턴으로 구현해 코드 간결화, 커넥션 연결/해제와 같은 중복 코드를 제거했습니다.

public interface UserRepository {
    Long save(User user);

    Optional<User> findById(Long userId);

    void clear();

    boolean existsByName(String username);

    Optional<User> findByUsernameAndPassword(String username, String password);

    void delete(User user);

    List<User> findAll();
}
@Repository
public class UserPersistenceRepository implements UserRepository {

    private final JdbcTemplate jdbcTemplate;
    private final RowMapper<User> rowMapper;

    private UserPersistenceRepository(
        DatabaseConfiguration dbConfig,
        RowMapper<User> rowMapper
    ) throws IOException {
        this.jdbcTemplate = dbConfig.jdbcTemplate();
        this.rowMapper = rowMapper;
    }

    @Override
    public Long save(User user) {
        String sql = insert();
        return jdbcTemplate.queryForObject(connection -> {
            PreparedStatement pstmt = connection.prepareStatement(sql, RETURN_GENERATED_KEYS);
            pstmt.setString(1, user.getUsername());
            pstmt.setString(2, user.getPassword());
            pstmt.setTimestamp(3, Timestamp.valueOf(user.getCreatedAt()));
            pstmt.setTimestamp(4, (user.getLastModifiedAt() != null) ? Timestamp.valueOf(user.getLastModifiedAt()) : null);
            pstmt.setObject(5, user.getDeleted().toString());
            pstmt.executeUpdate();
            try (ResultSet generatedKeys = pstmt.getGeneratedKeys()) {
                if (generatedKeys.next()) {
                    return generatedKeys.getLong(1);
                } else {
                    throw new DataAccessException();
                }
            }
        });
    }

    @Override
    public Optional<User> findById(Long userId) {
        String sql = selectBy(User.class);
        return jdbcTemplate.queryForObject(sql, pstmt ->
                pstmt.setLong(1, userId),
            rs -> rs.next() ? ofNullable(rowMapper.mapRow(rs)) : empty()
        );
    }

    @Override
    public boolean existsByName(String username) {
        String sql = selectBy(User.class, "username");
        return jdbcTemplate.queryForObject(sql, pstmt ->
                pstmt.setString(1, username),
            rs -> rs.next() && rs.getInt(1) > 0
        );
    }

    ....

}






✨ 트랜잭션

우리가 스프링에서 사용하는 서비스는 사실 Proxy로 한 번 감싸진 상태인데요, 이를${SERVICE} Proxy 클래스를 만들어 트랜잭션을 구현했습니다.

@Slf4j
@Component
public class UserServiceProxy implements UserSaveUseCase, UserSearchUseCase, UserDeleteUseCase {

    private final PlatformTransactionManager txManager;
    private final UserService target;

    public UserServiceProxy(
        DatabaseConfiguration dbConfiguration,
        UserService target
    ) throws IOException {
        this.txManager = dbConfiguration.transactionManager();
        this.target = target;
    }

    @Override
    public User save(User user) {
        TransactionStatus txStatus = getTransactionStatus(false);
        log.info("txStatus:[{}]", txStatus.getTransaction());
        try {
            User newUser = target.save(user);
            txManager.commit(txStatus);
            return newUser;
        } catch (Exception exception) {
            txManager.rollback(txStatus);
            throw new RuntimeException();
        }
    }

    .....

}






이를 통해 데이터의 정합성을 보장하도록 했으며, PlatformTransactionManager을 통한 추상화로, 구현체에 영향을 받지 않도록 했습니다.

image

- 각 모듈 간 의존관계를 명시하는 README.md
- .gitignore에 추가
- mvc 모듈에 jdbc 의존성 추가
- jackson databind
- DBCP, MySQL 의존성 추가
- 핸들러에서 프록시를 호출하도록 프록시 클래스 추가
- MySQL을 사용해 데이터를 조회하도록 구현체 교체
- 기본 생성자 추가
- delete 메서드 위치 이동
- 사용하지 않는 registerId 메서드 제거
- 핸들러 진입 전/후 로그를 남기기 위해 어노테이션 추가
- 프록시를 구현체로 등록하기 위해 ApplicationContext 리팩토링
@devjun10 devjun10 added ⚙️ refactor 의사결정이 있는 코드 수정 🚄 feat 기능 개발, 모듈 구현, 기능 개발을 위한 코드 추가 📝 docs 문서 작성 💻 chore 의존성 주입, 빌드 labels Feb 5, 2024
@devjun10 devjun10 requested a review from J-MU February 5, 2024 17:12
@devjun10 devjun10 self-assigned this Feb 5, 2024
}

@Override
@SneakyThrows
Copy link

Choose a reason for hiding this comment

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

@SneakyThrows 를 사용하신 이유가 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

개발할 때, 빠르게 구현하고 고치려고 @SneakyThrows 를 사용했습니다. 이를 붙이지 않으면 try/catch로 처리하거나, 상위로 예외를 던져야 하기 때문입니다. 이 부분은 제출 기한이 늦어져서 일단 구현하고 나중에 고치려 했습니다. 연관 코드 수정해서 제출하겠습니다.

log.info("Set auto-commit false.");
prepareConnectionForTransaction(connection, definition);

bindResource(definition.getId(), txObject);
Copy link

Choose a reason for hiding this comment

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

여기서도 connection을 release해줘야하지 않나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메서드는 트랜잭션을 시작하는 메서드기 때문에 release할 필요가 없어요.

public final class TransactionSynchronizationManager {

private TransactionSynchronizationManager() {
throw new AssertionError("올바른 방식으로 생성자를 호출해주세요.");
Copy link

Choose a reason for hiding this comment

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

private으로 생성자를 선언했으면 애초에 호출할 수가 없을텐데(컴파일 타임에서 잡힐텐데)
내부에서 throw하도록 만들어놓으신 이유가 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리플렉션으로 생성자를 호출할 수 있기 때문에 이를 방지하고자 예외를 던졌습니다.

Copy link

Choose a reason for hiding this comment

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

리플렉션이 좀 더 활발히 사용되는 "프레임워크"를 개발 중이기에 사용하신 방법인가요? 가능성이 거의 없는 행위까지 하나하나 막아야만 한다고 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

방어적으로 코드를 작성하는 것은 좋은 습관이라고 생각하기에, 작성하는게 좋다고 생각합니다.

사실 해당 코드를 작성하는데 얼마 걸리지 않는데, 작성하지 않을 이유가 없다고 생각합니다. 👀

Copy link

@kmularise kmularise left a comment

Choose a reason for hiding this comment

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

트랜잭션 구현까지 고생하셨습니다!


### VS Code ###
.vscode/
*.yml

Choose a reason for hiding this comment

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

.yml을 .gitignore에 추가해놓은 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

외부에 config 서버를 두고 설정 파일을 다운받아 사용할 예정이기에 yml 파일이 노출될 필요가 없기 때문입니다.

image

Choose a reason for hiding this comment

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

프로필을 분리해서 관리하는� 방식은 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그것도 괜찮은 방식이죠. 👍 기존에 스프링을 사용할 때, 말씀하신 개발 환경에 따른 분리를 하기 때문에 이번 미션에선 다른 방법을 채택했습니다.


implementation("com.fasterxml.jackson.core:jackson-databind:2.13.1")
implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.13.1")
}

Choose a reason for hiding this comment

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

추가하신 각각의 의존성에 대해서 설명해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jackson-databind는 자바 객체 <-> Json 객체를 변환해 주는 의존성입니다. 이를 통해 자바 객체를 Json 데이터로 직렬화/역 직렬화할 수 있습니다. 그런데 코드를 제출하고 보니, 굳이 해당 의존성은 사용할 필요는 없을 것 같네요.


jackson-dataformat-yaml는 yaml 데이터 형식을 객체로 직렬화/역 직렬화 해주는 의존성입니다. ConfigMap을 통해 .yml 파일을 객체로 만들고 있는데요, 이를 통해 MySQL 데이터베이스와 연동하기 위함입니다.

  1. yml파일을 자바 객체에 바인딩
  2. 설정 정보를 통해 MySQL과 연동
  3. 데이터베이스 연동

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다시 생각해보니 jackson-databind는 사용할 필요가 없겠네요. 👀 제외하겠습니다.


@Slf4j
@Component
public class UserLoginServiceProxy implements UserLoginUseCase {

Choose a reason for hiding this comment

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

프록시 패턴을 사용하신 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

스프링의 트랜잭션과 유사하게 구현하기 위해, 데이터의 일관성과 무결성을 유지하기 위해 Controller에서 실제 target이 아닌 프록시를 호출했습니다. 아래 메서드에서 비즈니스 로직이 완벽히 실행되지 않으면 commit이 아닌 rollback이 발생하며, 이를 통해 모든 데이터가 롤백 됩니다.

@Slf4j
@Component
public class UserServiceProxy implements UserSaveUseCase, UserSearchUseCase, UserDeleteUseCase {

    ......

    @Override
    public User save(User user) {
         // 트랜잭션 시작
        TransactionStatus txStatus = getTransactionStatus(false);
        log.info("txStatus:[{}]", txStatus.getTransaction());
        try {
            User newUser = target.save(user);

            // 모든 로직이 성공했을 때 commit.
            txManager.commit(txStatus);
            return newUser;
        } catch (Exception exception) {
            txManager.rollback(txStatus);
            throw new RuntimeException();
        }
    }
    
    ......

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.

프록시 패턴과 데코레이터 패턴의 차이점에 대해 설명해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

프록시를 생성하는 것은 컴파일 시점이 맞고, 이를 사용하는 것은 런타임 시점이 맞다고 생각하는데, 제가 놓친 다른 질문 의도가 있으실까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

프록시 패턴과 데코레이터 패턴은 글이 길어질 것 같아 블로그 링크로 대체하겠습니다.

Copy link

@kmularise kmularise Feb 13, 2024

Choose a reason for hiding this comment

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

JDK Dynamic Proxy와는 다르게 프록시를 동적으로 생성하지 않는 이유가 있나요?

서킷브레이커 관련 내용은 처음 알아갑니다! 도움이 많이 되었습니다!

Session findSession = target.login(username, password);
txManager.commit(txStatus);
return findSession;
} catch (Exception exception) {

Choose a reason for hiding this comment

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

RuntimeException이 아닌 Exception은 잡는 이유가 있을까요?

@@ -0,0 +1,4 @@
dependencies {
implementation("org.apache.commons:commons-dbcp2:2.10.0")
implementation("com.mysql:mysql-connector-j:8.1.0")

Choose a reason for hiding this comment

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

각각의 의존성에 대해서 설명해주세요!

String password
) throws SQLException {
String sql = new StringBuilder("SELECT * FROM ")
.append(convertCamelToSnake(clazz.getSimpleName()))

Choose a reason for hiding this comment

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

SQL 주입을 방지하기 위해서 어떤 처리를 하고 계신가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 구현하면서 이 부분이 걸렸는데, 현재는 아무런 대응이 돼 있지 않습니다. PpreparedStatement를 사용하면 SQL Injection을 막을 수 있지만, 이것까지 구현하기엔 시간이 부족했습니다. 그래도 기본적인 방어는 하기 위해 DDL 명령어는 검사하도록 반영하겠습니다.

@JohnPrk
Copy link

JohnPrk commented Feb 6, 2024

3주차 미션 진행 중이신가요? 혹시 3주차 미션에 대한 요구사항이 README.md에 어느 부분에 있나요?

@devjun10
Copy link
Contributor Author

devjun10 commented Feb 6, 2024

3주차 미션 진행 중이신가요? 혹시 3주차 미션에 대한 요구사항이 README.md에 어느 부분에 있나요?

아직 README.md에 요구사항을 정리해서 못 올렸네요. 올리겠습니다.

@JohnPrk
Copy link

JohnPrk commented Feb 6, 2024

JdbcTransactionManager의 계층도의 각 클래스를 분리시켰는지 알 수 있을까요? 클래스가 가지고 있는 책임과 역할에 대해서 알려주시면 좋을꺼 같습니다.

- MySQL 의존성을 app 모듈로 이동
- jackson-bind 의존성 app 모듈에서 제거
- JdbcTemplate Helper 클래스 추가
- 응답을 WRITE 에서 처리하도록 리팩토링
* 아직 미완성
- 코드 스멜 제거
- 에러가 발생했을 때, 로그를 확인할 수 있도록 Proxy에 로그 추가
- static끼리 어떤 객체가 먼저 생길지 순서가 정해지지 않아 이슈 발생.
}

private static Map<DataSource, Object> getTransactionMap() {
if (factory.get() == null) {

Choose a reason for hiding this comment

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

이전과 비교해서 코드의 로직은 달라지지 않은건가요? 리팩토링이라고 되어 있어서 여쭤봅니다.

추가로 null 값 고려 좋은 거 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

로직은 달라지지 않았고, factory가 null일 때 초기화하기 위해 해당 코드를 넣었습니다.

조금 지나서 기억이 가물가물한데, static을 여러곳에서 사용해 초기화 순서에 영향이 있어 이렇게 구현했습니다.

SocketChannel socketChannel = serverChannel.accept();
socketChannel.configureBlocking(false);
socketChannel.register(selector, SelectionKey.OP_READ);
socketChannel.register(selector, OP_READ);

Choose a reason for hiding this comment

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

사소한 부분이긴 한데 static import로 하면 어떤 점이 좋나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

static import를 사용하면 코드가 간결해지고, 명확해진다는 장점이 있습니다.

@@ -1,3 +1,4 @@
dependencies {
implementation("org.reflections:reflections:${reflectionsVersion}")
implementation("com.fasterxml.jackson.core:jackson-databind:2.16.1")

Choose a reason for hiding this comment

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

이 의존성을 추가하신 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 글로 작성하면 길어지고 이해도 잘 안될 것 같아 구두로 설명하겠습니다. 🙏


@Override
@SneakyThrows
public boolean existByName(String username) {
Copy link

Choose a reason for hiding this comment

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

exist -> exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영 완료.

import java.sql.SQLException;

public abstract class AbstractPlatformTransactionManager implements PlatformTransactionManager {

Copy link

Choose a reason for hiding this comment

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

따라서 인터페이스는 has-a 관계이며, 추상 클래스는 has-a 관계입니다.

  1. 해당 부분 실수하신거같은데 다시 설명해주실 수 있나요?
  2. 공통기능이 정의되어있지 않다고 말씀하셨는데 getTransaction메서드는 공통기능 아닌가요?

public abstract class AbstractPlatformTransactionManager implements PlatformTransactionManager {

@Override
public final TransactionStatus getTransaction(TransactionDefinition definition) throws SQLException {
Copy link

Choose a reason for hiding this comment

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

해당 메서드도 자식 클래스에서만 사용될텐데 왜 public으로 작성하셨나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

자바 인터페이스에 선언된 메서드의 접근 수준은 줄일 수 없으니까요. 아래 인터페이스에서 메서드는 기본적으로 public입니다. 그러면 자식에서 그 이하의 접근 제어자를 못 쓰겠죠?

public interface PlatformTransactionManager extends TransactionManager {
    TransactionStatus getTransaction(TransactionDefinition definition) throws SQLException;

    void commit(TransactionStatus status);

    void rollback(TransactionStatus status);
}

return new DefaultTransactionStatus(transaction, true, false);
}

abstract void doBegin(Object transaction, TransactionDefinition definition) throws SQLException;
Copy link

Choose a reason for hiding this comment

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

해당 메서드는 왜 default접근제어자를 사용하셨나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메서드는 제가 구현했지만 실제로는 스프링 프레임워크가 제공해주는 메서드입니다. 따라서 불필요하게 public으로 모두 오픈해서 다른 곳에서 사용할 필요가 없다고 판단해 접근 제어자를 default로 설정했습니다.

public final class TransactionSynchronizationManager {

private TransactionSynchronizationManager() {
throw new AssertionError("올바른 방식으로 생성자를 호출해주세요.");
Copy link

Choose a reason for hiding this comment

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

리플렉션이 좀 더 활발히 사용되는 "프레임워크"를 개발 중이기에 사용하신 방법인가요? 가능성이 거의 없는 행위까지 하나하나 막아야만 한다고 생각하시나요?


try (ResultSet rs = pstmt.executeQuery()) {
if (rs.next()) {
return rs.getInt(1) > 0;
Copy link

Choose a reason for hiding this comment

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

ResultSet의 getInt를 통해 가져온 데이터가 있는지를 확인하고 계시는데..

getInt 지정한 칼럼의 값을 int타입으로 읽어오는 메서드입니다.
image

ResultSet의 메서드 중에서는 getRow라는 메서드가 존재하고 이는 현재 커서가 가리키고 있는 row가 몇번째인지를 나타냅니다.
image

물론 동작은 하겠지만 getInt보다는 getRow를 사용하는게 메서드의 의도에 더 부합한다고 저는 느껴지는데 getInt를 사용하신 이유가 있으신가요?

참고 링크 : document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

더 상위에서 try/catch로 예외를 처리하기 때문입니다. 즉, 해당 부분에 try/catch가 없더라도, 더 상위 메서드로 가서 try/catch 내에서 처리되기 때문에 불필요한 try/catch문을 제거했습니다.

이 부분은 제가 몰라서 저렇게 작성했습니다. 새로 하나 배워갑니다. 👍

return entity;
}

public T findById(
Copy link

Choose a reason for hiding this comment

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

  1. findByUsernameAndPassword에서는 Optional을 통해 반환하고 여기서는 Optional을 사용하지 않는데 특별한 이유가 있으신가요?
try (ResultSet rs = pstmt.executeQuery()) {
                if (rs.next()) {
                    T entity = clazz.getDeclaredConstructor().newInstance();
                    Field[] fields = clazz.getDeclaredFields();
                    for (Field field : fields) {
                        if (isStatic(field.getModifiers()) || field.isSynthetic()) {
                            continue;
                        }
                        field.setAccessible(true);
                        Object value = rs.getObject(convertCamelToSnake(field.getName()));
                        if (value != null) {
                            Object convertedValue = convertValueToFieldType(rs, field);
                            field.set(entity, convertedValue);
                        }
                    }
                    return entity;
                } else {
                    return null;
                }
            } catch (InvocationTargetException e) {
                throw new RuntimeException(e);
            }
        }

해당 로직은 데이터를 가져올 때 마다 반복적으로 나타나는 로직인거같은데 공통로직으로 뺄 순 없을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 이전 구현을 보고 있으신 것 같습니다. 저 부분은 공통으로 한 번 묶었다가, 구현 전체가 바뀌었습니다.


public class ConfigMap {

private SpringConfig spring;
Copy link

Choose a reason for hiding this comment

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

필드명을 springConfig라고하지 않고 spring이라고 하신 이유가있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yml의 파일의 설정 구조를 따라기 때문에 spring으로 변수명을 지었습니다.

spring: 
    ......


public class ConnectionHolder {

private String uuid;
Copy link

Choose a reason for hiding this comment

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

ConnectionHolder에 id가 필요했던 이유가 뭔가요? 해당 id가 어디서 쓰이죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 id는 커넥션을 구분하기 위해 제가 임의로 설정해 놓은 값입니다.

}

public static void doReleaseConnection(Connection connection) throws SQLException {
if (connection == null) {
Copy link

Choose a reason for hiding this comment

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

connection이 없으면 return을 해주기보단 예외를 반환하는게 더 적절했을거 같은데 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어떤 방법도 상관 없다고 생각하는데, connection을 해제하는 부분에서는 최대한 방어적으로 개발하려고 했습니다. 어떤 순간에도 작업이 끝나면 커넥션은 끊어져야 하니까요.

private Deleted deleted;

public User() {
}
Copy link

Choose a reason for hiding this comment

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

실제 jackson이 직렬화 하는 방법에는 기본생성자 + getter말고도 몇 가지 다른 방법이 있다고 알고있습니다.
해당 방법들을 아는 만큼 나열해주세요.

.isInstanceOf(RuntimeException.class)
.isExactlyInstanceOf(UserNotFoundException.class)
.hasMessage("사용자를 찾을 수 없습니다.");
}
Copy link

Choose a reason for hiding this comment

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

테스트 이름은 "삭제된 사용자를 삭제하려면 UserNotFoundException이 발생한다" 인데 작성된 코드는 삭제된 사용자를 조회하는 테스트 아닌가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그렇네요. 이 부분 반영하겠습니다.

package project.server.app.test.integrationtest;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.AfterEach;
Copy link

Choose a reason for hiding this comment

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

마땅히 질문할 위치가 없어 여기서 질문합니다.
통합테스트를 RestAssured와 같은 라이브러리를 통해 요청을 만들어서 테스트 하는 것이 아닌 Service단에서 부터 테스트 하시는 이유가 무엇인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 구두로 설명했기 때문에 넘어가도록 하겠습니다.

} catch (Exception exception) {
throw new RuntimeException();
}
return target.findSessionById(userId);
Copy link

Choose a reason for hiding this comment

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

try-catch문을 제거하신 이유가 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

더 상위에서 try/catch로 예외를 처리할 수 있기 때문입니다. 즉, 해당 부분에 try/catch가 없더라도, 더 상위 메서드로 가서 처리되기 때문에 불필요한 부분을 제거했습니다.

}

findUser.delete(LocalDateTime.now());
userRepository.delete(findUser);
Copy link

Choose a reason for hiding this comment

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

  1. delete메서드에서는 findById를 통해서 이미 삭제된 유저인지를 먼저 확인해준 다음에 이미 삭제된 유저라면 UserNotFoundException을 던지고, 존재하면서 삭제되지 않은 유저인지를 확인한 후에만 삭제를 시켜주고 있습니다.
    사실 정상적인 경로의 요청이라면 위의 validation에는 걸리지 않을 것 같아요. 비 정상적인 경로의 요청을 위해 DB에 조회 요청을 날리는 것이 바람직하다고 생각하시나요?
    조회 요청을 하지 않아도 DB에 유저가 존재하지 않는다면 던저지는 예외를 이용하여 존재하지 않는 유저의 경우는 예외를 던저줄 수 있을테고, 이미 삭제된 유저에 대해서 UserNotFoundException을 던져주기 위해 삭제 flag가 되어있는지를 확인할 필요가 있나 의문이 듭니다.

  2. User의 delete는 로그인한 유저가 스스로 회원 탈퇴하는 경우에만 사용되는 메서드입니다. 그런 경우라면 delete보다는 withdraw와 같은 이름을 사용하는 것이 메서드의 목적을 더 잘 나타냈지 않을까 생각이 드는데 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

개발자 도구(F12)를 보면 서버 URL, 혹은 리버스 프록시의 IP 주소가 나옵니다. 악의적인 마음을 먹는다면 충분히 시도할 수 있기 때문에 이에 대한 대비를 해야 한다고 생각합니다. 한 번의 실수가 원하지 않는 방향으로 프로그램을 동작시킬 수도 있으니까요. 개인적으로 개발자가 애플리케이션을 꽉 쥐고 있어야 한다고 생각하기에, 사소하지만 이런 부분이 중요하다고 생각합니다.

Copy link
Contributor Author

@devjun10 devjun10 Feb 15, 2024

Choose a reason for hiding this comment

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

User의 delete는 로그인한 유저가 스스로 회원 탈퇴하는 경우에만 사용되는 메서드입니다. 그런 경우라면 delete보다는 withdraw와 같은 이름을 사용하는 것이 메서드의 목적을 더 잘 나타냈지 않을까 생각이 드는데 어떻게 생각하시나요?

저는 프로그램을 만드는 것은 개발자다보니 개발자 입장에서 생각했는데, 반대로 사용자 입장에서 생각할 수 있겠네요. 다만 이미 구현이 돼 있고, 크게 신경쓰이는 부분은 아니라 그대로 둘 생각입니다.

Copy link

Choose a reason for hiding this comment

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

개발자 도구(F12)를 보면 서버 URL, 혹은 리버스 프록시의 IP 주소가 나옵니다. 악의적인 마음을 먹는다면 충분히 시도할 수 있기 때문에 이에 대한 대비를 해야 한다고 생각합니다. 한 번의 실수가 원하지 않는 방향으로 프로그램을 동작시킬 수도 있으니까요. 개인적으로 개발자가 애플리케이션을 꽉 쥐고 있어야 한다고 생각하기에, 사소하지만 이런 부분이 중요하다고 생각합니다.

정상적인 경로외에는 생각할 필요가 없다라기 보다는 비정상적인 경로의 요청을 위해 DB Call(GET User findById)를 하고있으신데, 어차피 유저가 존재하지 않는다면 DB단에서 예외를 던질테니 그 예외를 받아 처리해도 될거같다고 생각해요. 해당 관점에서의 답변을 듣고싶습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그러면 결국 데이터베이스까지 요청이 전달 되는데, 비정상적인 요청은 그 전에 처리가 돼야 한다고 생각합니다. 데이터베이스까지 도달한 것은 애플리케이션에서 적절한 처리를 못한 것이 되니까요. 또한 불필요한 커넥션이 맺어져 자원 낭비도 발시닝하게 됩니다.

@spring-server spring-server deleted a comment from devjun10 Feb 15, 2024
@Controller
public class LoginController implements Handler {

private static final String MAX_AGE = "; Max-Age=900";

Choose a reason for hiding this comment

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

Max-Age는 무엇인가요? Max-Age를 컨트롤러에서 관리하시는 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

max-age는 자원이 유효한 최대시간을 말합니다. 이를 통해 인증/인가되지 않은 사용자가 서버에 접근할 경우 세션을 초기화 합니다. 이를 컨트롤러/Validator에서 관리하는 것은 아직 인터셉터를 구현하지 않았기 때문입니다.

서비스는 비즈니스 로직을 처리하기 때문에 이 과정이 진행되기 전, 인증/인가가 모두 처리 돼야 한다고 판단했고, 따라서 컨트롤러에서 이를 처리하고 있습니다.

@Component
public class UserValidator {

private static final String CACHE_CONTROL = "cache-control";

Choose a reason for hiding this comment

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

cache-control, max-age와 같은 스태틱 상수들은 무엇을 의미하나요?
UserValidator에서 관리하시는 이유가 궁금합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

위 답변과 겹치는 부분이 있어서 Cache-Control에 관해서만 설명하겠습니다. Cache-Control은 캐시를 제어하기 위해 사용하는 헤더입니다. 여기에는 더 많은 옵션이 존재하지만, 저는 max-age만 사용했는데, 세션이 올바르지 않을 경우, 해당 세션을 제거하기 위해서입니다. max-age는 해당 캐시를 최대 얼마 동안 보관하는지에 대한 설정인데, 이 값을 0으로 설정할 경우 캐싱하지 않게 됩니다.

Cache-Control 헤더를 사용하지 않았을 경우, 브라우저가 로컬 캐시를 사용해 개인정보 조회 페이지로 이동 자체를 하지 하지 않는 문제 가 있었습니다. 즉, 개인 정보를 조회하다가 에러가 발생하면 index.html 페이지로 리다이렉트 시키는데, 그 전에 에러가 발생했다면 이를 캐싱하고 있다가 올바른 페이지로 가야 함에도 계속해서 index.html로 리다이렉트 시키는 것입니다. 이를 막기 위해 Cache-Control로 캐시를 제거해버렸습니다.

  1. 처음에 에러가 발생. index.html로 리다이렉트.
  2. 올바른 접근을 해도 기존에 캐싱한 값을 토대로 무조건 index.html로 리다이렉트

UserValidator에서 이 값을 내려주는 게 조금 아쉬운데, 원래는 인터셉터에서 이를 처리하고 싶었습니다. 다만 이 부분까지 구현하기는 시간이 좀 모자라서 다음 스텝에서 진행하기 위해 남겨두었습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 chore 의존성 주입, 빌드 📝 docs 문서 작성 🚄 feat 기능 개발, 모듈 구현, 기능 개발을 위한 코드 추가 ⚙️ refactor 의사결정이 있는 코드 수정 🐝 test 테스트코드 작성

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants