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

refact: API 스키마 및 에러 핸들링 개선 #38

Merged
merged 13 commits into from
Aug 9, 2024

Conversation

minkyu97
Copy link
Collaborator

@minkyu97 minkyu97 commented Aug 8, 2024

기존 코드의 문제점

기존에는 hodu core 의 채점 결과로부터 응답 스키마를 생성하는 코드가 너무 장황하고 boilerplaty 했다.
그리고 채점 중 에러 발생 시 에러를 캐치하지 않아서 요청이 abort 되었다.

리팩토링 방법

serde 의 다양한 속성들을 이용하여 응답 스키마 생성 로직을 단순화한다.
그리고 futures 크레이트를 추가하여 채점 중 발생한 에러를 핸들링 할 수 있도록 한다.

관련 링크

https://serde.rs/remote-derive.html
https://doc.rust-lang.org/std/panic/struct.AssertUnwindSafe.html
https://docs.rs/futures/latest/futures/future/trait.FutureExt.html#method.catch_unwind

@minkyu97 minkyu97 requested a review from woohm402 August 8, 2024 15:35
@minkyu97 minkyu97 self-assigned this Aug 8, 2024
@@ -17,15 +21,17 @@ async fn submit_code(
submission.language
);

let output = mark(MarkParams {
let output = AssertUnwindSafe(mark(MarkParams {
Copy link
Collaborator Author

@minkyu97 minkyu97 Aug 8, 2024

Choose a reason for hiding this comment

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

주의) 장기적으로는 futures::FutureExt::catch_unwindstd::panic::AssertUnwindSafe 를 걷어내고 hodu-core 의 응답을 Result 로 감싸는 것이 좋겠습니다.

Dockerfile.dev Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Dockerfile.local 은 어때요? dev 는 약간 dev 환경에 대해 써야 되는 단어같아서요!

@minkyu97
Copy link
Collaborator Author

minkyu97 commented Aug 9, 2024

어우 힘들어

@minkyu97 minkyu97 merged commit fb3da6b into main Aug 9, 2024
1 check passed
@minkyu97 minkyu97 deleted the refactor/api-schema-and-error-handling branch August 9, 2024 05:30
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.

2 participants