ArunPrakashG / wordpress_client

A powerful and easy-to-use WordPress REST API client for Dart & Flutter.
MIT License
32 stars 12 forks source link

Should I capture the DioException? Or should the library map them? #68

Open MiniSuperDev opened 2 months ago

MiniSuperDev commented 2 months ago

There are several cases in which it happens, I will show you 2

Case 1: for invalid port

try {
  final client =
      WordpressClient(baseUrl: Uri.parse('https://localhost:686868'))
        ..initialize();
  final request = await client.posts.listRaw(
    ListPostRequest(),
  );
} catch (exception, stackTrace) {
  print(stackTrace);
  print(exception);
}
DioException [unknown]: null
Error: Invalid argument(s): Invalid port 686868

#0      DioMixin.fetch (package:dio/src/dio_mixin.dart:509:7)
<asynchronous suspension>
#1      InternalRequester.execute (package:wordpress_client/src/internal_requester.dart:212:22)
<asynchronous suspension>

Case 2: Port not available

try {
  final client =
      WordpressClient(baseUrl: Uri.parse('https://localhost:9999'))
        ..initialize();
  final request = await client.posts.listRaw(
    ListPostRequest(),
  );
} catch (exception, stackTrace) {
  print(stackTrace);
  print(exception);
}
DioException [connection error]: The connection errored: The remote computer refused the network connection.
 This indicates an error which most likely cannot be solved by the library.
Error: SocketException: The remote computer refused the network connection.

#0      DioMixin.fetch (package:dio/src/dio_mixin.dart:509:7)
<asynchronous suspension>
#1      InternalRequester.execute (package:wordpress_client/src/internal_requester.dart:212:22)
<asynchronous suspension>

I think is because you don't catch exceptions here, but I'm not sure when you map it.

https://github.com/ArunPrakashG/wordpress_client/blob/455d26c4f2d480598dfd171e1f1e124641e3fe2a/lib/src/internal_requester.dart#L190-L212

Thanks

ArunPrakashG commented 2 months ago

Hey @MiniSuperDev I think the URI class should throw the exception and not Dio when the parsed port value is not in a valid range. Seems like it doesn't detect it while parsing. I will add a check for the same on the codebase.

I have added few validations on the constructor to validate the entered URL.

Also, the code you shared on the internal_requester is fine as-is i belive, i have also updated it to rethrow the exception from there. The errors are not caught there for the main request is because as it is an async function, the exception is propagated to it's caller, which in here is the endpoint methods in the internal_requester_base.dart file. the exception is handled in this layer as required.

Thank you!

ArunPrakashG commented 2 months ago

@MiniSuperDev The changes are published on v8.4.6. Do give it a try and let me know!

MiniSuperDev commented 2 months ago

@ArunPrakashG Thank you for the quick response.

I think it would be better if instead of throwing an ArgumentError you caught it and returned the WordpressFailureResponse, just like you do with Socket Exception, It is a similar case, since although the port is valid, it does not mean that it exists, it is only known in runtime.

In my case the user enters the url, and if you do not capture it and return an WordpressFailureResponse, you are forcing the user to use the map and also a try catch.

try {
  final client = WordpressClient(
      baseUrl: Uri.parse('https://localhost:2229/wp-json/wp/v2')) // use invalid port to force to catch the exception
    ..initialize();
  final request = await client.posts.list(
    ListPostRequest(),
  );
  request.map(onSuccess: (response) {
    print(response);
  }, onFailure: (response) {
    print(response);
  });
} catch (exception, stackTrace) {
 // For argument errors, maybe others exceptions if you don't catch all and map it in WordpressFailureResponse
  print(stackTrace);
  print(exception);
}
WordpressResponse(code: -2, headers: {}, duration: 0:00:00.000000, message: DioException [connection error]: The connection errored: The remote computer refused the network connection.
 This indicates an error which most likely cannot be solved by the library.
Error: SocketException: The remote computer refused the network connection.
 (OS Error: The remote computer refused the network connection.
, errno = 1225),

Well I think that in general you should not throw any exceptions if you are returning a Result Type (Success/Failure), since it would be necessary to use both a try catch and a map or switch.

I don't know if you don't catch some other exception, but the idea is that if you use a result type, the user doesn't have to worry about catching something. It would be another case if instead of a result type you threw exceptions, but in the same way it would be there too. map everything to a domain exception set in this case WordpressException so as not to catch anything else. So you should catch all and in ErrorType you can have unknown for the exceptions that you don't know. Like the ones I showed, I know that there are some other exceptions that are not caught (unusual, but in other projects I have handled them).

Well, it's my opinion, I hope it helps you and if you have any comments, I'm here.


On the other hand, this does not work for all cases.

I found it because I'm testing some sites that each path can be a full site.

https://domain.com/path-N/site-M/wp-json/wp/v2

And also some that change the endpoint for a custom one, domain.com/api/

https://github.com/ArunPrakashG/wordpress_client/blob/91d6f5fa6c7ff94d1b2fa5ece57652be9b65058e/lib/src/utilities/helpers.dart#L143

However if you map to WordpressFailureResponse these validations are not necessary


Likewise, thank you for creating a good library :)

ArunPrakashG commented 2 months ago

@MiniSuperDev Thank you for your feedback and suggestions.

INVALID PORT

I think it is the duty of the developer to NOT push invalid ports into production. Because this is a very serious case and this is not something which we should handle with a try-catch. If the port is invalid, then the entire functionality won't work. (Depending on the functionality)

In case you are writing something which creates multiple instances of the client and run some sort of tests on sites, then I think it is better if you add some logic to validate port, along with the host before connecting to it. One way to do it is by pinging the host. If you want to proceed with the client itself to validate the site is made with WordPress, then you could use the staticmethod on WordpressClient by calling isWordpressSite(Uri(...));

In my opinion, Exceptions should be thrown by libraries as a way of communicating about crucial problems in the codebase which may interfere with the working of the functionality itself. and the Failure response model contains data related to exceptions which are not entirely breaking the app. i.e., Some exceptions should be handled during development itself while some we could handle during runtime like showing a dialog etc.

Additionally, it is important to note that some exceptions cannot always be represented into the WordpressFailureResponse category. only the exceptions related to the request is mapped to the WordpressFailureResponse

PATH VALIDATION

I had thought about such situation when writing it. However, I proceed with it because remapping the path of the REST API, which comes by default on WordPress now is very rare, even if such case exists, it is a common standard to have /wp-json/ as start for the discovery. In the future, I may have to rewrite it, surely. If you have suggestions on the same, please share!

Do share your thoughts on these!

MiniSuperDev commented 2 months ago

Hello Exceptions are fine, but the problem in my opinion is that you are mixing 2 different types of way to handle exceptions or failures, the result type and the exception. (On the internet you can see several about result vs exception and avoid mixing them up.) The exception is the standard in dart, but if you use a result type, throw exception should be avoided, maybe throw error. errors vs exceptions in dart like InitializationError

It would be good for the library to have only 1, or throw only exceptions or return the result type.


The invalid port and the host is mostly because in my case the users are the ones who put the URLs of the websites, they can put invalid ports, or wrong domains, in general the dio exception indicate what is wrong.

I could validate it before as your said or I could catch the exception that the http client gives, but it would be simpler with a single type of error handling (exception or result).

In the event that you chose only result, it would be ideal to catch exceptions that the http client may throw in a possible error, or if you go only for exceptions indicate which exceptions it can throw apart from wordpress exception if your don't map it.


And the invalid path is because the first path param is no ever the first path segment.

In this case 3 different sites in each path, that is an alternative to subdomains.

https://domain.com/site-1/wp-json/wp/v2
https://domain.com/site-2/wp-json/wp/v2
https://domain.com/site-3/wp-json/wp/v2

Thanks

ArunPrakashG commented 2 months ago

@MiniSuperDev I just tried out the code you provided on previous comment on the latest build. It seems to work fine for me, with the invalid port error being wrapped inside the Failure response when using map method. I didn't use any try-catch here.

final wpClient = WordpressClient(
    baseUrl: Uri.parse(
      'https://localhost:2229/wp-json/wp/v2',
    ),
  ) // use invalid port to force to catch the exception
    ..initialize();

  final request = await wpClient.posts.list(
    ListPostRequest(),
  );

  log('Request: ${request.isSuccessful}');

  final status = request.map(
    onSuccess: (response) {
      return true;
    },
    onFailure: (response) {
      log(response.error.toString());
      return false;
    },
  );

  log('Status: $status');

I think this is what your described right? This was the expectation when I worked on the WordpressSuccessResponse and WordpressFailureResponse. Errors are thrown for parameters which are invalid. that is unchanged I don't intent on changing that. Could you please confirm this on the latest build?

Additionally, if you really need to catch the ArgumentErrors, you can wrap the initialization logic inside guard(() => ...); on the latest release.


The reason I suggested you to validate the port etc beforehand is to minimize the memory allocation when creating a whole client and it's interfaces just to catch an error. I think it is a much safer approach.

ArunPrakashG commented 1 month ago

@MiniSuperDev Hey, any updates here? were you able to get the expected results?

MiniSuperDev commented 1 month ago

@ArunPrakashG Hi Checkout with raw methods

import 'package:wordpress_client/wordpress_client.dart';

void log(dynamic message) {
  print(message);
}

void main() async {
  final wpClient = WordpressClient(
    baseUrl: Uri.parse(
      'https://localhost:2229/wp-json/wp/v2',
    ),
  )..initialize();

  final request = await wpClient.posts.listRaw(
    ListPostRequest(),
  );

  log('Request: ${request.isSuccessful}');
}
DioException [connection error]: The connection errored: The remote computer refused the network connection.

Also websites is sub paths don't pass, like:

'https://localhost:2228/site-1/wp-json/wp/v2',

https://github.com/ArunPrakashG/wordpress_client/blob/7f9afb2efa911e760a096abc22d69d7746c18f6c/lib/src/utilities/helpers.dart#L143 The first path segment is no ever wp-json, you can asume the 3 from end to start if there is not custom endpoint

Thanks

ArunPrakashG commented 1 month ago

Hey @MiniSuperDev, I have made the changes and published it on v8.4.7. Please try it out and let me know!

As for the second case, I changed this uri.pathSegments.first == 'wp-json'; to uri.pathSegments.contains('wp-json'); such that it is a hard requirement to have wp-json in the REST API path. this is as per Wordpress standards and i don't think it is a good idea to remove this requirement.