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

Interceptors not called if converters throws an exception #167

Closed
danielgomezrico opened this issue Aug 23, 2020 · 14 comments
Closed

Interceptors not called if converters throws an exception #167

danielgomezrico opened this issue Aug 23, 2020 · 14 comments
Labels
wontfix This will not be worked on

Comments

@danielgomezrico
Copy link
Contributor

danielgomezrico commented Aug 23, 2020

Currently if theres an exception from the converters then the interceptors are not called, it does not print the response.

    chopper = ChopperClient(
      ...
      converter: JsonConverter(),
      interceptors: [
        HttpLoggingInterceptor()
      ],
    );

You just get a trace like the next one but it is hard to debug what was the answer that resulted into this:

E/flutter ( 6262): [ERROR:flutter/lib/ui/ui_dart_state.cc(166)] Unhandled Exception: type '_InternalLinkedHashMap<String, dynamic>' is not a subtype of type 'SessionResponse'
E/flutter ( 6262): #0      JsonConverter.decodeJson (package:chopper/src/interceptor.dart:226:46)
E/flutter ( 6262): #1      JsonConverter.convertResponse (package:chopper/src/interceptor.dart:231:12)
E/flutter ( 6262): #2      ChopperClient._decodeResponse (package:chopper/src/base.dart:165:29)
E/flutter ( 6262): #3      ChopperClient._handleSuccessResponse (package:chopper/src/base.dart:247:17)
E/flutter ( 6262): #4      ChopperClient.send (package:chopper/src/base.dart:300:19)

But you dont get the print from the HttpLoggingInterceptor.

@JEuler
Copy link
Collaborator

JEuler commented Sep 8, 2020

Need to investigate this. Thx!

@Jparrgam
Copy link

Hola @danielgomezrico bro eso me pasaba a mi, la solucion a eso fue crear un mapa de funciones para llamar al parser del json a la clase que necesitas ejemplo

final Map<Type, Function> typeToJsonFactoryMap;

en el metodo convertResponse del converter hago algo como:

return response.copyWith( body: fromJsonData<BodyType, InnerType>( response.body, typeToJsonFactoryMap[InnerType]), );

el metodo fromJsonData es

T fromJsonData<T, InnerType>(String jsonData, Function jsonParser) { if (jsonData.isJson()) { var jsonMap = json.decode(jsonData); if (jsonMap is List) { return jsonMap .map( (item) => jsonParser(item as Map<String, dynamic>) as InnerType) .toList() as T; } return jsonParser(jsonMap); } return null; }
y en el chopper module

RequestJsonConverter( SessionResponse: (jsonData) => SessionResponse.fromJson(jsonData), )

@stewemetal stewemetal added bug Something isn't working investigation The issue needs further investigation labels Jan 15, 2021
@stewemetal
Copy link
Collaborator

If I get it correctly, Interceptors not being called is not the real problem here.

First, we should focus on making the exception more informative.

@danielgomezrico
Copy link
Contributor Author

@stewemetal that's true in part, if the exception can have the full response and the parts that fail it would be awesome, I just thought that it will be good to see the response with the headers and all of the prints that the interceptors prints will add value too.

@danielgomezrico
Copy link
Contributor Author

danielgomezrico commented Jan 15, 2021

@Jparrgam gracias pero no agarre tu idea, los ejemplos no son claros, puedes desarrollar la idea mejor? depronto si lo haces en ingles quedaría más accesible para el resto.

@techouse
Copy link
Collaborator

techouse commented Apr 5, 2024

@Guldem was this fixed in #547?

@Guldem
Copy link
Contributor

Guldem commented Apr 6, 2024

I doesn't solve the cause of the exception. But I added a test see #600 to verify that exception are passed up in the interceptor chain.

This doesn't mean that all the interceptors are "called" and "executed" but could theoretically catch these exception and do something.

Interceptor that can catch the exception are only called for the request and receive the exception instead of the response on the way back I guess.

@danielgomezrico
Copy link
Contributor Author

@techouse @Guldem the issue is not that the interceptor throws an exception but that if a converter throws an exception then no interceptors are executed.

I think this is still not solved

@techouse techouse reopened this Jul 3, 2024
@Guldem
Copy link
Contributor

Guldem commented Jul 4, 2024

I understand the Interceptor are not throwing the exception but I don't understand what is expected when a exception is thrown by the Converter.

I don't think it matter where the exception is thrown. If something is thrown either in the converter or the interceptor you need to deal/resolve the exception in order to continue. We cannot throw the exception and continue with the converter or interceptor.

If you want to print information about the response even though there is a exception this can also be done in the converter I guess.

Can you provide us with a example of what you want to achieve?

@danielgomezrico
Copy link
Contributor Author

The use that breaks to me is that:

  • I have a JSON converter that throws an exception when the body does not match the expected serializer
  • I have an interceptor that logs all the requests

When the converter fails, I cannot see in the logs the response body, so I can fix it, I just see a big the exception saying I was expecting a String and got null. It is not enough to understand what else is bad, and so I was expecting to be able to see the logs from the interceptor so see the full body of the request

@Guldem
Copy link
Contributor

Guldem commented Jul 4, 2024

Thanks for elaborating more on the issue. I don't think there's much we can do. One way I can think of is providing a way to insert a "logging" interceptor before the converters. But I think that would make the library less developer friendly and also creates other openings for weird behaviour if used incorrectly(what will happen with developers 😉 ).

Another way to get closer to the actual calls, could be by providing a custom HttpClient (wrapper) that does the logging for you. This way you sort of skip the chopper stuff regarding response handling.

I also don't necessarily thing this is a bug. The exception is there for a reason and needs to be solved. If the exception is not from Chopper itself its hard for us to clarify whats happing. For chopper and interceptor to work, the converter needs to resolve this in order to continue. Ofcourse we can try to provide tools to make debugging and inspecting easier. But in that case there are also third party tools available. For example using dart dev tool and the http inspector or regular debugging.

@techouse Do you have any other ideas or insights?

@techouse
Copy link
Collaborator

techouse commented Jul 5, 2024

One way I can think of is providing a way to insert a "logging" interceptor before the converters.

This should be logged in Crashlyitics anyway.

I also don't necessarily thing this is a bug. The exception is there for a reason and needs to be solved.

Agreed. Your service call fails because of bad data and your app should not display bad data. For example, it's wiser to show an error screen than an incorrect bank account balance to the user.

@Guldem pretty much summed it up. Either you provide a custom http.Client or do some magic in your JSON converter.

Closing this issue.

@techouse techouse closed this as completed Jul 5, 2024
@danielgomezrico
Copy link
Contributor Author

Excuse me 🙈 I disagree with the idea that this is not a bug, I know that it could be difficult to fix but that's another conversation.

Think about this other use case (which I lived in the past, and now I realize that we could have a bug if this happens):
Let's say that each request responds with a new token every time a request is done (which is a possible configuration for auth workflows on the backend), and so you could write an interceptor that will refresh your local token on every request so you always send a valid token. If this fails because the parsing didn't work, then the hole auth will be broken and the client will have to reauthenticate again.

This idea of creating a hole Client for this may work, but it is not the responsibility of Chopper itself, and that's one of the reasons of why the interceptors exist?

@techouse techouse added wontfix This will not be worked on and removed bug Something isn't working investigation The issue needs further investigation labels Jul 7, 2024
@Guldem
Copy link
Contributor

Guldem commented Jul 9, 2024

Yes that indeed why interceptors exist, only with the requirement that the request can be converted and the flow doesn't throw errors. The other idea behind chopper is that it can automatically convert requests to a specific type. Which causes a conflict.

Regarding your example. I think this is a very specific use case, which can also be solve by providing the custom client. I also think that the converter should not need to throw parsing exceptions if implemented correctly. front-end & back-end should implement the same interface. Especially in valid requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants