birdofpreyru / react-native-fs

File system access for React Native
https://dr.pogodin.studio/docs/react-native-file-system
Other
160 stars 12 forks source link

uploadFiles() doesn't return error status code or message in the promise catch()-method. #48

Closed Crare closed 3 months ago

Crare commented 4 months ago

I have tried to handle file upload in the backend and return error in some business case validation, but I can't get the error result to show in the uploadFiles catch-method. It only gives same generic error everytime no matter what:

I've changed the url. but upload works, but error handling is non existent. I can't handle and show specific error to the user why their upload didn't work.

[Error: ENOENT: no such file or directory, open 'http://url/api/upload']

and on JSON.stringify:

err { "nativeStackAndroid": [], "userInfo": null, "message": "ENOENT: no such file or directory, open 'http://url/api/upload'", "code": "ENOENT" }

import * as RNFS from '@dr.pogodin/react-native-fs';

// some other code for the component...

// inside the component on a call function:
RNFS.uploadFiles({
    toUrl: 'http://url.com/api/upload',
    files: files, // RNFS.UploadFileItemT[]
    method: 'POST',
    headers: {
        Accept: 'application/json',
        Authorization: `Bearer ${accessToken}`,
    },
    fields: {
        // form data
        Foo: 'Bar',
    }
})
    .promise.then(response => {
        console.log(response);
    })
    .catch(err => {
        console.log(err);
        console.log('err', JSON.stringify(err, null, ' '));
    })

// rest of the component...
birdofpreyru commented 4 months ago

Hmm... if look through the native code, uploadFiles() does have at least some code to catch and transmit actual errors; and if you search for the no such file or directory, open string in the codebase, you'll find it in multiple methods, but not in uploadFile() (perhaps uploadFile() depends on some of that methods that do throw this error). So... not sure what happens in your case, and how your toUrl value ends up in this error message (my first though would be a bug, but the example app has tests for uploadFile() that work fine).

It should be investigated, but uploadFiles() is not something I personally rely upon, thus a low priority for me, thus probably I won't look into it anytime soon.

Crare commented 4 months ago

Thanks for looking into it. I did too. I found that in the onUploadComplete() row 757 in file ReactNativeFsModule.kt, it goes there in the reject()-method, which leads to rejectFileNotFound()-method and then to that message: "no such file or directory, open". I tried this by changing the values in every step to see where it leads. It would be useful if the error would return the statuscode all the way to the JS-code. But I checked that too, it looked like status code was 0. But should have been in my case 422 (Unprocessable Content), which is what our server sends in specific case we want to handle and a error-message comes there too from backend. This error happens after the files have been loaded to the server.

birdofpreyru commented 4 months ago

Thanks for your donation @Crare , I'll mention you as a sponsor in the repo's README sometime later.

I believe the file/line you mention just propagates the error message, but does not generate it; and looking at the code, I guess, it is supposed that if it is just a server-side error there should be no exception there, it will just go the first branch of if-condition that do include statusCode into the result.

The actual error should be coming from here, meaning the upload() method there throws the exception: https://github.com/birdofpreyru/react-native-fs/blob/b45a9c23fa3899bafe2fcdfa056fa334b3d0ffa6/android/src/main/java/com/drpogodin/reactnativefs/Uploader.kt#L26-L32 I think, the easiest to debug will be if you just build & run your app in debug mode via Android Studio, set a breakpoint at the beginning of that upload() method, and go through it step-by-step, to see at what point it throws and why. The error reads like it fails when handling local file access, rather than because server responded with error code.

Crare commented 4 months ago

Hi! I found where the problem is finally by debugging that uploadFiles()-method in Android Studio. The problem is that the connection.inputStream is throwing exception and there is available connection.errorStream. Using the errorStream as responseStream I was able to get the error response from server to native code. And looks like it is now not going to reject(), as it is not adding the response to exception. but anyway it allows me to get the response result to JS-code which is helpful. I could add exception there when errorStream is not null at the end of Uploader.kt after it gets statusCode at the bottom of the upload()-method, but in that case the exception that is returned in promise.reject doesn't currently contain any statuscodes or error messages from server response.

Should I make a PR for this or do you have time to look into it? This was just a fix to Android side. I need to check iOS side too.

change:

if (connection.errorStream != null) {
    responseStream = BufferedInputStream(connection.errorStream)
} else {
    responseStream = BufferedInputStream(connection.inputStream)
}
responseStreamReader = BufferedReader(InputStreamReader(responseStream))
Crare commented 4 months ago

not sure if it should throw exception and go to promise.reject in that case if statusCode is not successful type or just check the result statuscode in the JS-code side.

the example handles the statusCode in the promise.resolve:


// upload files
RNFS.uploadFiles({
  toUrl: uploadUrl,
  files: files,
  method: 'POST',
  headers: {
    'Accept': 'application/json',
  },
  fields: {
    'hello': 'world',
  },
  begin: uploadBegin,
  progress: uploadProgress
}).promise.then((response) => {
    if (response.statusCode == 200) {
      console.log('FILES UPLOADED!'); // response.statusCode, response.headers, response.body
    } else {
      console.log('SERVER ERROR');
    }
  })
  .catch((err) => {
    if(err.description === "cancelled") {
      // cancelled by user
    }
    console.log(err);
  });
birdofpreyru commented 4 months ago

Ok, I see, I was wrong in my previous message, if server responds with an error status code the attempt to access input stream throws, as you say, and the library rejects the uploadFiles() promise.

Here are details from HttpURLConnection docs: Screenshot from 2024-05-31 11-50-09

Great if your little patch resolves it for you, but for the library I guess it needs somewhat extra work and more complex solution: I guess, it should keep the past behavior and reject JS-side promise on such error, but it should indeed expose additional details from errorStream. So, perhaps on Native side it should do what you did, but then on JS side to check the status code in the resolved native promise, and depending on that to resolve or reject the caller-facing JS promise. That's my first impression, I should think about it more carefully. Then, tests for this updated behavior should be added to the example app, to ensure it works as intended on Android, and that it also works consistently for library user across Android/iOS/Windows; and lastly the README documentation should be updated to keep it clear how it works.

birdofpreyru commented 4 months ago

I don't quite have time to seriously look into all this right now, and presumably I'll be busy with higher priority stuff the next few weeks. I'd say, perhaps, use https://www.npmjs.com/package/patch-package to patch it directly in your project, copy/paste the patch into here, so I can refer to it whenever I have time, and then sometime later I probably look into adopting it into the library, perhaps with some modifications of the behavior, but keeping the actual functionality you need.

Crare commented 4 months ago

I checked iOS-side now, it already works in that way that it returns resolve with the statusCode and message in server error cases. So now they would be consistent with iOS and Android side. I don't know how to test the Windows-side.

birdofpreyru commented 4 months ago

Cool! Then it makes the life easier... still will need to ensure it is covered by tests in the example app, and correctly documented in README.