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

graphql #50

Merged
merged 6 commits into from
Jun 6, 2023
Merged

graphql #50

merged 6 commits into from
Jun 6, 2023

Conversation

eyakauleva
Copy link
Owner

close #49

@eyakauleva eyakauleva self-assigned this May 31, 2023
@eyakauleva eyakauleva requested a review from h1alexbel May 31, 2023 15:24
Repository owner deleted a comment from codecov-commenter May 31, 2023
@@ -43,4 +54,69 @@ public WebClient.Builder webClient() {
return WebClient.builder();
}

public GraphQLScalarType localDateTimeScalar() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva try to make more elegant solution here

Copy link
Owner Author

Choose a reason for hiding this comment

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

done 679c337

import org.springframework.stereotype.Component;

@Component
public class ExceptionResolver extends DataFetcherExceptionResolverAdapter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva nice solution

since GraphQL always throws 200 HTTP status code it's very inconvenient to show errors to the end user

Copy link
Owner Author

Choose a reason for hiding this comment

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

@h1alexbel GraphQL still returns 200 HTTP status code. This resolver sets status code only as a field to a response body and also sets custom error message there.
As I found out, it's a GraphQL standard to always return 200 OK in such cases, cause non-200 response codes indicate that some issue occurred at the HTTP transport layer, not the GraphQL layer.
So I'm not sure whether it's a good practice to replace GraphQL HTTP status code and whether it's possible at all.


@MutationMapping("createUser")
public Mono<EsUser> create(@Argument final User user) {
CreateUserCommand command = new CreateUserCommand(user, "Liza123");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva what is the "Liza123"?

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed 679c337

}).build();
}

public ValidationSchemaWiring schemaWiring() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva as far I understand this ValidationSchemaWiring also should be represented as Spring Bean

Copy link
Owner Author

Choose a reason for hiding this comment

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

@h1alexbel I call this method in another method, then why it should be a bean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva for code reuse?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@h1alexbel but I call this method only once

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva I know, but what about future usages?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@h1alexbel according to YAGNI principle features should only be added when required

Copy link
Collaborator

@h1alexbel h1alexbel Jun 1, 2023

Choose a reason for hiding this comment

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

@eyakauleva YAGNI is about features or FR, not about maintainability, which is NFR

try to design all your classes, methods, functions with maintainability in mind

Copy link
Owner Author

Choose a reason for hiding this comment

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

done 679c337


@MutationMapping("deleteUser")
public Mono<EsUser> delete(@Argument final String id) {
DeleteUserCommand command = new DeleteUserCommand(id, "Liza123");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyakauleva the same goes here

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed 679c337

@eyakauleva eyakauleva requested a review from h1alexbel June 2, 2023 14:23
Repository owner deleted a comment from codecov-commenter Jun 2, 2023
@h1alexbel h1alexbel merged commit db47ac4 into master Jun 6, 2023
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.

graphql
2 participants