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

웹소켓 채팅 리팩터링 #160

Merged
merged 6 commits into from
Dec 26, 2024
Merged

웹소켓 채팅 리팩터링 #160

merged 6 commits into from
Dec 26, 2024

Conversation

thdwoqor
Copy link
Contributor

@thdwoqor thdwoqor commented Dec 4, 2024

코드에 코멘트 작성

@Override
public void configureClientInboundChannel(ChannelRegistration registry) {
registry.interceptors(stompChannelInterceptor);
registry.interceptors(
Copy link
Contributor Author

@thdwoqor thdwoqor Dec 4, 2024

Choose a reason for hiding this comment

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

코드 작성하면서 생각하다보니깐 제일 바꾸고 싶었던 영역은 이부분인거같아

private static final String SUBSCRIBE_FORMAT = "%s/%s";
private static final String PUBLISHING_FORMAT = "%s/%s/%s";

기존에는 인터셉터를 통해서 모든 요청에 대해서 위와 같은 URL 패턴이아니면 에러가 발생해서 저런 형태가 아닌 방식으로 pub/sub을 할때 문제가 생겼는데 PathMatcher 를 통해서 필요한 부분만 등록해주는게 필요하다고 생각했어

이전에 확장성 떨어진다고 말한부분이 이부분 이였어
현재 구조에서는 모든요청이 해당 인터셉터를 통과하는 문제

Copy link
Contributor

Choose a reason for hiding this comment

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

기존에는 인터셉터를 통해서 모든 요청에 대해서 위와 같은 URL 패턴이아니면 에러가 발생해서 저런 형태가 아닌 방식으로 pub/sub을 할때 문제가 생겼는데 PathMatcher 를 통해서 필요한 부분만 등록해주는게 필요하다고 생각했어

이 말이 구독을 위해서는 "sub/{code}/"이 형식이어야하고 발행은 "pub/{code}/{name}"이 형식이 어색하다는거고 이걸 PatchMatcher의 채인 메서드로 필요할때마다 추가해서 확장성을 높였다는 의미지?

난 이런 의미면 좋은거 같아

Copy link
Contributor Author

Choose a reason for hiding this comment

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

형식이 어색한것도 있지만 큰문제는

  1. 현재 구독,발행을 위해서 "sub/?/{auth}/" , "pub/?/{auth}" 형식이 강제 되는상황인데 이유는 인터셉터가 전역으로 설정되어있어서 그럼 ("sub/{auth}/hello" , "pub/bye" 이렇게 불가능)
  2. 그래서 인터셉터를 구독할때 현재처럼 전역으로 설정하는게아니라 PatchMatcher를 통해서 특정 url, 메서드에만 적용범위를 좁혀서 확장성을 높임 (sub/{auth}/hello, "pub/bye" 이런 형식으로 다양한 방식으로 여러 인터셉터 적용가능)

Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 기가막힌데??
구우욷!

약간 이 패턴과 COMMAND에 걸리면 우리가 만든 Interceptor 로 보내주는거지?

    private final Map<String, ChannelInterceptor> channelInterceptors;
    private final PathMatcher pathMatcher;
    private final List<StompMapping> includePathPattern;
    private final List<StompMapping> excludePathPattern;

    public record Record(
            String key,
            ChannelInterceptor channelInterceptor
    ) {
    }

    public PathMatcherInterceptor(
            final Record... records
    ) {
        channelInterceptors = Arrays.stream(records).collect(
                Collectors.toMap(
                        Record::key,
                        Record::channelInterceptor
                )
        );
        this.pathMatcher = new AntPathMatcher();
        this.includePathPattern = new ArrayList<>();
        this.excludePathPattern = new ArrayList<>();
    }
...

그렇다면, 뭔가 이런 느낌으로 만들어주고, includePath 할 때 특정 키워드 넣어가지고 어떤 키워드에 매칭 되었을 때, channel Interceptor 에 할당할지도 결정하면 좋을 것 같아!

Copy link
Contributor

@waterricecake waterricecake left a comment

Choose a reason for hiding this comment

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

고생했어 파워!

다 좋은데 이건 어떤가하는 부분이 있어서 코멘트 남겼어

@Override
public void configureClientInboundChannel(ChannelRegistration registry) {
registry.interceptors(stompChannelInterceptor);
registry.interceptors(
Copy link
Contributor

Choose a reason for hiding this comment

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

기존에는 인터셉터를 통해서 모든 요청에 대해서 위와 같은 URL 패턴이아니면 에러가 발생해서 저런 형태가 아닌 방식으로 pub/sub을 할때 문제가 생겼는데 PathMatcher 를 통해서 필요한 부분만 등록해주는게 필요하다고 생각했어

이 말이 구독을 위해서는 "sub/{code}/"이 형식이어야하고 발행은 "pub/{code}/{name}"이 형식이 어색하다는거고 이걸 PatchMatcher의 채인 메서드로 필요할때마다 추가해서 확장성을 높였다는 의미지?

난 이런 의미면 좋은거 같아

Copy link
Collaborator

@kpeel5839 kpeel5839 left a comment

Choose a reason for hiding this comment

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

고생했어

간단하게 코멘트 남겼어잉

@Override
public void configureClientInboundChannel(ChannelRegistration registry) {
registry.interceptors(stompChannelInterceptor);
registry.interceptors(
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 기가막힌데??
구우욷!

약간 이 패턴과 COMMAND에 걸리면 우리가 만든 Interceptor 로 보내주는거지?

    private final Map<String, ChannelInterceptor> channelInterceptors;
    private final PathMatcher pathMatcher;
    private final List<StompMapping> includePathPattern;
    private final List<StompMapping> excludePathPattern;

    public record Record(
            String key,
            ChannelInterceptor channelInterceptor
    ) {
    }

    public PathMatcherInterceptor(
            final Record... records
    ) {
        channelInterceptors = Arrays.stream(records).collect(
                Collectors.toMap(
                        Record::key,
                        Record::channelInterceptor
                )
        );
        this.pathMatcher = new AntPathMatcher();
        this.includePathPattern = new ArrayList<>();
        this.excludePathPattern = new ArrayList<>();
    }
...

그렇다면, 뭔가 이런 느낌으로 만들어주고, includePath 할 때 특정 키워드 넣어가지고 어떤 키워드에 매칭 되었을 때, channel Interceptor 에 할당할지도 결정하면 좋을 것 같아!

@thdwoqor thdwoqor merged commit 501f1d4 into dev Dec 26, 2024
1 check passed
@thdwoqor thdwoqor deleted the feat/159 branch December 26, 2024 06:39
waterricecake added a commit that referenced this pull request Dec 26, 2024
* feat: 웹소켓 채팅 리팩터링

* feat: 채팅 롤백 Path 롤백 및 인터셉터 수정

* refactor: job skill 수정

* refactor: auth 인증부분 분리

* fix: WebsocketPlayerArgumentResolver 패키지 위치 수

---------

Co-authored-by: waterricecake <[email protected]>
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.

3 participants