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

엔드포인트 리팩토링 #63

Open
justinyoo opened this issue Jul 30, 2022 · 19 comments
Open

엔드포인트 리팩토링 #63

justinyoo opened this issue Jul 30, 2022 · 19 comments
Labels
oca-issue Issue that Open-source Contribution Academy involves in

Comments

@justinyoo
Copy link
Contributor

Requirement:

  • 현재 각 엔드포인트 안에서 일어나는 일련의 워크플로우를 Fluent API 형태로 정리할 것
@juijeong8324
Copy link

의정팀이 담당하겠습니다!

@justinyoo justinyoo added the oca-issue Issue that Open-source Contribution Academy involves in label Aug 12, 2022
@justinyoo
Copy link
Contributor Author

우선 엔드포인트 하나만 해 보세요: https://github.com/devrel-kr/nhn-toast-notification-service-custom-connector/blob/main/src/nt-sms/Triggers/GetMessage.cs

대강의 슈도 코드는 대충 이런 느낌?

try
{
    var payload = this._workflow
                      .ValidateHeader(req)
                      .ValidateQuery(req)
                      .BuildRequestUrl<GetMessageRequestPaths>(this._settings)
                      .Invoke();

    return new OkObjectResult(payload);
}
catch (Exception ex)
{
    return new BadRequestResult();
}

@wnsgml7267
Copy link

해당 이슈를 해결함에 있어 제가 이해한 것이 맞는지 멘토님께 확인하고 싶습니다.
nt-sms 폴더에 있는 api들의 모든 메소드가 동일한 워크플로우를 가지고 있는데, 그 부분이 헤더, 쿼리, requestUrl 이고, 해당 부분을 따로 서비스 레이어를 생성하여 만든 후 그에 따른 테스트 코드도 수정하는 것으로 이해했습니다. 제가 이해한 것이 맞는지 궁금합니다.

@justinyoo
Copy link
Contributor Author

맞습니다. 제가 위에 적어놓은 수도 코드를 보면 this._workflow 라는 게 보이죠? 이 부분이 WorkflowService 비슷한 서비스 레이어를 담당한다고 보면 됩니다. 따라서, 이렇게 코드가 바뀐다면 이에 맞게 기존 테스트 코드를 수정하고, 새 테스트 코드를 도입해야 할 겁니다.

@wnsgml7267
Copy link

멘토님, 리팩토링에 어려움을 겪고 있어서 도움을 요청해보려 합니다..
서비스 레이어에 동일한 워크플로우 부분을 넣기 위해 docs.microsoft.dotnet/C# 레퍼런스를 참고하면서 문법이나 개체 지향 프로그래밍 부분 등을 공부하고 있지만, 실질적으로 응용해서 적용하는 데에 어려움을 겪고 있습니다. 어떤식으로 넣어줘야 하는지 계속 고민해봐도 감이 잘 잡히지 않습니다.
일단 제일 처음으로 ValidateHeader부분만 해보려고 시도를 해보았습니다. 해당 메소드의 역할이 헤더 부분을 가져와서 validation을 하는 것이라 생각하여 그에 알맞다고 생각하는 코드를 임의로 작성해보았습니다.

namespace Toast.Sms.Service
{
    public class SmsService
    {
        private RequestHeaderModel _validateHeaders;

        public SmsService ValidateHeader(RequestHeaderModel validateHeaders)
        {
            try
            {
                validateHeaders = req.To<RequestHeaderModel>(useBasicAuthHeader: true).Validate();
            }
            catch (Exception ex)
            {
                return new BadRequestResult();
            }
            this._validateHeaders = validateHeaders;

            return this;
        }
    }
}

서비스 레이어에 이런 식으로 코드를 작성을 하면 멘토님께서 알려주신 Getmessage.cs의 수도코드에서 사용할 수 있지 않을까.. 해서 한 번 시도라도 해보려고 작성을 해보았습니다.

여기서 제가 궁금한 부분은 3가지가 있습니다.

1.리팩토링이 해당 방식과는 전혀 다른지.. 인터페이스(?)로 접근을 해야하는지 궁금합니다.
2.현재 Fork해서 수정해보고 있는데, 해당 api가 리팩토링이 제대로 되었는지 테스트를 해볼 수 있는 방법이 있다면 궁금합니다.
3.리팩토링 방법이 전혀 틀렸다면, 참고할만한 비슷한 로직의 서비스 레이어를 생성한 예제나 레퍼런스 등이 있는지 궁금합니다.

@juijeong8324
Copy link

juijeong8324 commented Aug 18, 2022

I have a question..

  • Should we make Workflow interface?
  • According to your sudo code, We should make ValidateHeader(), ValidateQuery(), BuildRequestUrl() and Invoke() in Workflow interface. Is it right what I understand?

@justinyoo
Copy link
Contributor Author

1.리팩토링이 해당 방식과는 전혀 다른지.. 인터페이스(?)로 접근을 해야하는지 궁금합니다.

리팩토링은 현재 GetMessage.cs 트리거 안의 로직이 그대로 서비스 레이어로 옮겨간다고 생각하시면 됩니다. 언급하신 코드는 살짝 다른 느낌이죠?

2.현재 Fork해서 수정해보고 있는데, 해당 api가 리팩토링이 제대로 되었는지 테스트를 해볼 수 있는 방법이 있다면 궁금합니다.

테스트 메소드를 만들어 보면 되겠죠? 이따 오피스 아워에서 잠깐 해 볼까요?

3.리팩토링 방법이 전혀 틀렸다면, 참고할만한 비슷한 로직의 서비스 레이어를 생성한 예제나 레퍼런스 등이 있는지 궁금합니다.

오피스 아워에서 한 번 다뤄 보시지욥.

@justinyoo
Copy link
Contributor Author

  • Should we make Workflow interface?

I would strongly recommend using the interface - the strategy pattern.

  • According to your sudo code, We should make ValidateHeader(), ValidateQuery(), BuildRequestUrl() and Invoke() in Workflow interface. Is it right what I understand?

That's correct.

@wnsgml7267
Copy link

테스트 중 오류가 생긴 부분에 대해 질문이 있습니다.
var config = new ConfigurationBuilder().AddJsonFile("test.settings.json").Build();
Run all Tests를 했을 때 해당 구문이 있는 모든 파일에 빨간 줄이 생기는 이유가 궁금합니다.

test settings json error

error 문구로 'test.settings.json' 구성 파일을 찾을 수 없다고 나옵니다. 실제 경로가 나와있어서 직접 보면 그 경로에 해당 파일이 없습니다.

원인이 무엇인지 알 수 있을까요? 제가 따로 건드린 코드는 없습니다.

@justinyoo
Copy link
Contributor Author

지금 폴더에 보면 test.settings.sample.json 파일이 있죠? 이 파일을 복사해서 test.settings.json 파일을 하나 추가해 보십쇼.

그리고, 이 클라스 위에 보면 [TestCategory("Integration")] 이라고 보일텐데, 이 테스트는 실제 API를 호출하는 경우라서 이건 별도의 방법을 통해 테스트 해야 합니다. 따라서, 테스트를 돌리실 때 아래 명령어를 사용하세요.

dotnet test . --filter:"TestCategory!=Integration"

@wnsgml7267
Copy link

wnsgml7267 commented Aug 20, 2022

멘토님께서 보여주신 validateheader부분을 리팩토링하는 과정을 복습하고, ValidateQuery리팩토링을 진행하다가 궁금한 점이 생겼습니다.

public class HttpTriggerWorkflow : IHttpTriggerWorkflow
    {
        private RequestHeaderModel _headers;

        /// <inheritdoc />
        public async Task<IHttpTriggerWorkflow> ValidateHeaderAsync(HttpRequest req)
        {
            var headers = req.To<RequestHeaderModel>(useBasicAuthHeader: true)
                             .Validate();

            this._headers = headers;

            return await Task.FromResult(this).ConfigureAwait(false);
        }
    }

해당 부분은 서비스 레이어를 구성하고 있는 HttpTriggerWorkflow.cs파일 중 일부입니다. 여기서 제가 이해한 바로는 리팩토링할 ValidateHeader, ValidateQuery, requestURL 로직들을 각각의 Task로 나누어서 해야할 것 같다는 생각이 들었습니다. 해당 방식이 맞을까요?
제가 생각한 방식대로 ValidateQuery 부분을 한 번 구성해보았습니다.

public class HttpTriggerWorkflow : IHttpTriggerWorkflow
    {
        private RequestHeaderModel _headers;
        private GetMessageRequestQueries _queries; 
        /// <inheritdoc />
        public async Task<IHttpTriggerWorkflow> ValidateHeaderAsync(HttpRequest req)
        {
            var headers = req.To<RequestHeaderModel>(useBasicAuthHeader: true)
                             .Validate();

            this._headers = headers;

            return await Task.FromResult(this).ConfigureAwait(false);
        }
        /// <inheritdoc />
        public async Task<IHttpTriggerWorkflow> ValidateGetMessageRequestQueryAsync(HttpRequest req)
        {
            var queries = await req.To<GetMessageRequestQueries>(SourceFrom.Query).Validate(this._validator)
                               .ConfigureAwait(false);
            this._queries = queries;

            return await Task.FromResult(this).ConfigureAwait(false);
        }

    }

이런식으로 Task를 추가하는 것이 맞을까요? 또한, 모든 api들에 있어서 동일한 Header와는 다르게 GetMessage, ListMessage 등... 각 api마다 query부분이 GetMessageRequestQueries, ListMessagesRequestQueries 이런식으로 다른데, 각각의 다른 query도 Task로 따로 따로 나누어야 하나요?

@wnsgml7267
Copy link

wnsgml7267 commented Aug 20, 2022

TestMethod를 생성하는 기준이 궁금합니다.

        public static RequestHeaderModel Validate(this RequestHeaderModel headers)
        {
            if (string.IsNullOrWhiteSpace(headers.AppKey))
            {
                throw new RequestHeaderNotValidException("Not Found") { StatusCode = HttpStatusCode.NotFound };
            }
            if (string.IsNullOrWhiteSpace(headers.SecretKey))
            {
                throw new RequestHeaderNotValidException("Unauthorized") { StatusCode = HttpStatusCode.Unauthorized };
            }

            return headers;
        }

제가 이해한 바로는 멘토님께서 ValidateHeader에 대한 TestMethod를 작성하실 때 RequestHeaderValidator.cs파일의 해당 코드를 참고하여 AppKey와 SecretKey를 확인하여 RequestHeaderNotValidException를 throw하는 것을 보고 작성하신 것으로 생각했습니다.
작성하신 TestMethod로는 header값이 없을 때, Null일 때, 둘 중 하나만 있을 때, 둘 다 있을 때(+private header확인)를 테스트 하신 걸로 이해했습니다.

    public static class GetMessageRequestQueryValidatorExtension
    {
        /// <summary>
        /// Validates the request query parameters against GetMessage.
        /// </summary>
        /// <param name="headers"><see cref="GetMessageRequestQueries"/> instance.</param>
        /// <returns>Returns the <see cref="GetMessageRequestQueries"/> instance.</returns>
        public static async Task<GetMessageRequestQueries> Validate(this Task<GetMessageRequestQueries> queries, IValidator<GetMessageRequestQueries> validator)
        {
            var instance = await queries.ConfigureAwait(false);

            var result = validator.Validate(instance);
            if (result.IsValid)
            {
                return instance;
            }

            throw new RequestQueryNotValidException("Invalid Query Parameters") { StatusCode = HttpStatusCode.BadRequest };
        }
    }

    /// <summary>
    /// this represents the validator entity for the GetMessage request query parameters.
    /// </summary>
    public class GetMessageRequestQueryValidator : AbstractValidator<GetMessageRequestQueries>
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="GetMessageRequestQueryValidator"/> class.
        /// </summary>
        public GetMessageRequestQueryValidator()
        {
            this.RuleFor(p => p.RecipientSequenceNumber).GreaterThan(0);
        }
    }

그렇다면 동일한 흐름으로 GetMessageRequestQueryValidator.cs파일에 존재하는 해당 코드를 보고 TestMethod를 작성해야겠다고 생각을 했습니다. 하지만, 해당 코드를 보고 TestMethod를 어떤식으로 몇 가지를 작성해야 할지 감이 잡히질 않습니다.
해당 방식으로 접근하는 것이 맞는지.. 아니라면 어떠한 기준으로 TestMethod를 작성해야 하는지 궁금합니다.

@justinyoo
Copy link
Contributor Author

해당 부분은 서비스 레이어를 구성하고 있는 HttpTriggerWorkflow.cs파일 중 일부입니다. 여기서 제가 이해한 바로는 리팩토링할 ValidateHeader, ValidateQuery, requestURL 로직들을 각각의 Task로 나누어서 해야할 것 같다는 생각이 들었습니다. 해당 방식이 맞을까요?

맞습니다.

제가 생각한 방식대로 ValidateQuery 부분을 한 번 구성해보았습니다.

public class HttpTriggerWorkflow : IHttpTriggerWorkflow
    {
        private RequestHeaderModel _headers;
        private GetMessageRequestQueries _queries; 
        /// <inheritdoc />
        public async Task<IHttpTriggerWorkflow> ValidateHeaderAsync(HttpRequest req)
        {
            var headers = req.To<RequestHeaderModel>(useBasicAuthHeader: true)
                             .Validate();

            this._headers = headers;

            return await Task.FromResult(this).ConfigureAwait(false);
        }
        /// <inheritdoc />
        public async Task<IHttpTriggerWorkflow> ValidateGetMessageRequestQueryAsync(HttpRequest req)
        {
            var queries = await req.To<GetMessageRequestQueries>(SourceFrom.Query).Validate(this._validator)
                               .ConfigureAwait(false);
            this._queries = queries;

            return await Task.FromResult(this).ConfigureAwait(false);
        }

    }

이런식으로 Task를 추가하는 것이 맞을까요? 또한, 모든 api들에 있어서 동일한 Header와는 다르게 GetMessage, ListMessage 등... 각 api마다 query부분이 GetMessageRequestQueries, ListMessagesRequestQueries 이런식으로 다른데, 각각의 다른 query도 Task로 따로 따로 나누어야 하나요?

쿼리 유효성 검사는 각 엔드포인트마다 다르기 때문에 이를 추상화 시키려면 제너릭을 추가하셔야 합니다. 예를 들자면...

public interface IHttpTriggerWorkflow
{
    Task<IHttpTriggerWorkflow> ValidateQueryAsync<T>(HttpRequest req);
}

이런 식으로 인터페이스를 구상하시면 좋겠죠?

@justinyoo
Copy link
Contributor Author

ValidateQueryAsync<T>() 메소드를 테스트할 때는 그 안에 실제 유효성 검사를 하진 않아요. 그게 목킹의 목적입니다. 목킹을 해서, 목킹 결과를 가정하는 거죠. 유효성 검사를 통과했을 경우 어떻게 되는가, 아닐 경우 어떻게 되는가를 테스트하면 됩니다.

@juijeong8324
Copy link

I'm asking you because I'm getting an error in refactoring the query.
(Query 부분을 리팩토링 하는데 계속 에러가 생겨 질문이 드립니다.)

Problem

        public async Task<IHttpTriggerWorkflow> ValidateQueryAsync<T>(HttpRequest req, IValidator<T> val) where T : BaseRequestQueries
        { 
            var queries = await req.To<T>(SourceFrom.Query).Validate(val).ConfigureAwait(false);

            this._queries = queries;

            return await Task.FromResult(this).ConfigureAwait(false);
        }

The workflow that we thought as follows
(저희가 생각한 workflow는 다음과 같습니다)

  1. Pass each Endpoints Query class(ex. GetMessageQueries) as Generic Type
    (각 엔트포인트 쿼리 클래스(ex. GetMessageQueries)를 Generic Type로서 전달)
  2. Pass IValidator as parameters
    (IValidator는 매개변수로 전달 )
  3. Call Validate(val) method.
    (Validate(val) 메소드를 호출)

image
However, The Error occurs at Validate(val) as above picture.
(그러나 **Validate(val)**에서 위와 같은 에러가 뜹니다. )


Code analysis

public static class GetMessageRequestQueryValidatorExtension
    {
        /// <summary>
        /// Validates the request query parameters against GetMessage.
        /// </summary>
        /// <param name="headers"><see cref="GetMessageRequestQueries"/> instance.</param>
        /// <returns>Returns the <see cref="GetMessageRequestQueries"/> instance.</returns>
        public static async Task<GetMessageRequestQueries> Validate(this Task<GetMessageRequestQueries> queries, IValidator<GetMessageRequestQueries> validator)
        {
            var instance = await queries.ConfigureAwait(false);

            var result = validator.Validate(instance);
            if (result.IsValid)
            {
                return instance;
            }

            throw new RequestQueryNotValidException("Invalid Query Parameters") { StatusCode = HttpStatusCode.BadRequest };
        }
    }

After checking the Validate method for each endpoint query, We have found that Validate method is Extension Method which have each Endpoint query instance as parameter.
(각 엔드포인트의 쿼리 Validate 메소드를 확인해본 결과 각 엔드포인트의 쿼리 인스턴스를 파라미터로 받는 확장 메소드였습니다. )


Question

  1. We thought we could use the extension method with the Generic Type, But why does the error keep occuring that the parameter was not delivered (see the picture at the top)?
    (Generic Type으로 확장 메소드를 사용할 수 있다고 생각했는데 왜 계속 파라미터가 전달되지 않았다고(맨 위의 사진 참고) 에러가 뜨나요?! )

  2. I wonder if it is the most efficient way to get the dependencies used by each endpoint as parameters.
    ( 각 엔드 포인트에서 사용되는 의존성들을 매개변수로 전달 받는 것이 가장 효율적인 방법인지 궁금합니다.)

@juijeong8324
Copy link

juijeong8324 commented Sep 5, 2022

Question

  1. We thought we could use the extension method with the Generic Type, But why does the error keep occuring that the parameter was not delivered (see the picture at the top)?
    (Generic Type으로 확장 메소드를 사용할 수 있다고 생각했는데 왜 계속 파라미터가 전달되지 않았다고(맨 위의 사진 참고) 에러가 뜨나요?! )

해당 이슈는 using Toast.Sms.Validators; 를 추가하니까 해결이 되었고

image
이제는 위와 같은 에러가 떴습니다.

이에 Validators 폴더에 멘토님이 알려주신 RequestQueryValidators.cs 클래스를 추가하니 (using Toast.Sms.Validators는 삭제 가능)

using System.Threading.Tasks;
using FluentValidation;

using Toast.Common.Models;

namespace Toast.Common.Validators
{
    public static class RequestQueryValidator
    {
        public static async Task<T> Validate<T>(this Task<T> queries, IValidator<T> validator) where T : BaseRequestQueries
        {
            return await queries.Validate(validator).ConfigureAwait(false);
        }
    }
}

해결되었습니다!

@wnsgml7267
Copy link

wnsgml7267 commented Sep 11, 2022

ValidateQueryAsync<T>() 메소드를 테스트하기 위해 유효성 검사를 통과했을 경우와 아닌 경우 두 가지로 나누어 테스트 코드를 작성해보면서 오류가 생겨서 질문 드립니다.

        //쿼리 유효성 검사 통과X
        [TestMethod]
        public void Given_ValidQueries_fails_When_Invoke_ValidateQueriesAsync_Then_It_Should_Throw_Exception()
        {
            var queries = new QueryString();
            queries.Add("Name","Value");
            //var queries = new BaseRequestQueries();
            
            var req = new Mock<HttpRequest>();
            req.SetupGet(p => p.QueryString).Returns(queries);
            
            //var validator = new RequestQueryValidator();
            var validator = new Mock<IValidator<BaseRequestQueries>>();
            var workflow = new HttpTriggerWorkflow();
            Func<Task> func = async () => await workflow.ValidateQueriesAsync<BaseRequestQueries>(req.Object, validator);

            func.Should().ThrowAsync<RequestQueryNotValidException>();
        }
        //쿼리 유효성 검사 통과o
        [DataTestMethod]
        [DataRow("hello", "world")]
        public async Task Given_ValidQueries_When_Invoke_ValidateQueriesAsync_Then_It_Should_Return_Result(string name, string value)
        {
            var queries = new QueryString();
            queries.Add("Name","Value");

            var req = new Mock<HttpRequest>();
            req.SetupGet(p => p.QueryString).Returns(queries);

            var validator = new Mock<IValidator<BaseRequestQueries>>();
            var workflow = new HttpTriggerWorkflow();

            var result = await workflow.ValidateQueriesAsync<BaseRequestQueries>(req.Object, validator);

            result.Should().BeOfType<HttpTriggerWorkflow>();
        }     
  1. queries에 뭘 담아야 하는지 궁금합니다.
    extensions
    예를 들어, 헤더 같은 경우에는 첨부한 사진을 참고하여 headers.Add("Authorization", $"Basic {encoded}"); 이런식으로 테스트 코드에 추가한 것을 볼 수 있는데 쿼리는 그런 부분을 찾을 수가 없어서 잘 모르겠습니다.

  2. 현재 전체 테스트 코드에서 ValidateQueryAsync<T>() 메소드의 파라미터에 validator만 에러가 나긴 하지만, 제네릭 부분과 validator를 정확히 어떤식으로 목킹해야 하는지 모르겠습니다.

@dhrudcks01
Copy link

image

HttpTriggerWorkflow.cs에서 Invoke를 이렇게 작성하였는데

테스트 코드를 돌려서 어떤값을 뱉어내야하는지 궁금합니다.
머리로는 되게 만든것 같은데 이런 방식이 맞는지도 궁금합니다.

image

@justinyoo
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
oca-issue Issue that Open-source Contribution Academy involves in
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants