Skip to content
This repository was archived by the owner on Mar 16, 2019. It is now read-only.

Error normalization (ongoing, tracking issue) #460

Open
lll000111 opened this issue Aug 5, 2017 · 6 comments
Open

Error normalization (ongoing, tracking issue) #460

lll000111 opened this issue Aug 5, 2017 · 6 comments

Comments

@lll000111
Copy link
Contributor

lll000111 commented Aug 5, 2017

I just wrote a function that renames a temporary file created by a stream to its final name (in my case the SHA-256 hash of the contents of the file is going to be the final name, so I don't know what it is until it's all there).

I use RNFetchBlob.fs.mv (link to fs.js).

That lead me to a hunt for the actual Java and iOS (Swift) methods used to rename the file (File.renameTo for Java, moveItemAtURL for iOS).

There is zero documentation of errors, and when I get down to it, I don't really know what the errors are that I'm going to get.

Some methods have a fixed string set in this project's source code - but a different one for Java and iOS versions? For example, promise.reject("ReadFile Error", err.getLocalizedMessage()); is used in readFile (Android) - but on iOS that string seems to be reject(@"RNFetchBlob failed to read file", err, nil); - and readStream has completely different error messages yet again...

By the way (see readStream, this function seems to have a single error message about "encoding" for everything incl. FileNotFound exceptions?
("Failed to convert data to "+encoding+" encoded string, this might due to the source data is not able to convert using this encoding.")

In my case I could not simply invoke the mv and handle an error - because I simply don't know what "destination file exists" errors are going to look like.

It would be nice to have documentation about (common) errors (like "file not found" for read file operations or "file exists" for write operations - and do they silently overwrite or not?) as well as a way to catch those common errors (fixed Error.name or Error.message (beginning of the string) independent of the platform.

Yeah, that could be a major effort :-( Just a suggestion.

It's of course possible to program without any such assurances, but then, in the specific example to have one, I have to do more I/O and use other functions to check for existence before the actual operation - which causes another issue because now I need to make the function "atomic", since a check for existence followed by access might be interrupted my another I/O event that creates the file in the meantime, even in single-threaded Javascript that's not safe. That's why it would be nice to be able to just do an operation and handle any errors later (like node.js philosophy for file operations).

wkh237 pushed a commit that referenced this issue Aug 9, 2017
* bump to 0.10.8

* Update PULL_REQUEST_TEMPLATE

* Fix #468 "Messy error returns: Sometimes a string, sometimes an Error object"

* Cleanup: remove an unused constant and duplicate method definitions

* Cleanup:
- fix minor errors in JSDoc comments, for example {string]} => {string}
- fix parameter name "encode" => "encoding" (more logical, and it says so in the function's JSDoc too)
- json-stream.js: split a looooong log message string constant into two parts and fix a typo ("maually"), and the type for objects is "Object" (capitalized) in Flow type annotations

* Fix a (Flow) type conflict - fixes issue #461

* NEEDS REVIEW - Attempt to fix some of issue #460 (Error message normalization)

Error messages reported by iOS and Android versions should be as similar as possible. Also, within the same system there should be consistency. This patch is an attempt to bring a LITTLE more of this consistency to the error messages. I also fixed some very few minor language issues, like "does not exist" (is the correct English). I tried keeping the changes to a minimum.

Background: In my project code I want to know when a file already exists (e.g. after calling fs.createFile), and the only way is to check the error message string that I get. It's bad if they differ between versions (createFileASCII and createFile) and then also between Android and iOS version. At least some core part of the string should be the same, so that I have something to match.

Ideally messages should come from a centralized easy (easier) to maintain file (for both iOS and Android), and ideally both systems should have the same errors and messages as far as possible.
@lll000111
Copy link
Contributor Author

lll000111 commented Aug 9, 2017

It's a little more equal now, but could still use more improvements, I'll leave this open. I'll work on it.

wkh237 pushed a commit that referenced this issue Aug 9, 2017
* bump to 0.10.8

* Update PULL_REQUEST_TEMPLATE

* Fix #468 "Messy error returns: Sometimes a string, sometimes an Error object"

* Cleanup: remove an unused constant and duplicate method definitions

* Cleanup:
- fix minor errors in JSDoc comments, for example {string]} => {string}
- fix parameter name "encode" => "encoding" (more logical, and it says so in the function's JSDoc too)
- json-stream.js: split a looooong log message string constant into two parts and fix a typo ("maually"), and the type for objects is "Object" (capitalized) in Flow type annotations

* Fix a (Flow) type conflict - fixes issue #461

* NEEDS REVIEW - Attempt to fix some of issue #460 (Error message normalization)

Error messages reported by iOS and Android versions should be as similar as possible. Also, within the same system there should be consistency. This patch is an attempt to bring a LITTLE more of this consistency to the error messages. I also fixed some very few minor language issues, like "does not exist" (is the correct English). I tried keeping the changes to a minimum.

Background: In my project code I want to know when a file already exists (e.g. after calling fs.createFile), and the only way is to check the error message string that I get. It's bad if they differ between versions (createFileASCII and createFile) and then also between Android and iOS version. At least some core part of the string should be the same, so that I have something to match.

Ideally messages should come from a centralized easy (easier) to maintain file (for both iOS and Android), and ideally both systems should have the same errors and messages as far as possible.

* Fixes #467 by improving the write() function of write streams: By resolving with the RNFetchBlobWriteStream instance instead of with "undefined" writes can now be chained:

RNFetchBlob.fs.writeStream(PATH_TO_FILE, 'utf8', true)
.then((ofstream) => ofstream.write('foo'))
.then((ofstream) => ofstream.write('bar'))
.then((ofstream) => ofstream.write('foobar'))
.then((ofstream) => ofstream.close())

Reference: #467 (comment)
@lll000111
Copy link
Contributor Author

@wkh237 I've prepared a commit that I still need to do at least basic tests on: https://github.com/lll000111/react-native-fetch-blob/commit/7258eb217e2024e68bcf35a043cbecf96ff8b9da

But you may already want to have a look.

@wkh237
Copy link
Owner

wkh237 commented Aug 12, 2017

@lll000111 , you can simply use our dev repo and run the test cases. I think that might be helpful 👍

@lll000111
Copy link
Contributor Author

lll000111 commented Aug 13, 2017

@wkh237 I don't see a file tests.js anywhere? The instructions in the test repo README include

Before install the test app, you should replace constants DROPBOX_TOKEN and TEST_SERVER_URL in src/test/tests.js

After cloning the submodule I ran find . -name tests.js and there is no such file anywhere.

I found it in ./test/test-init.js, I think the README (https://github.com/wkh237/react-native-fetch-blob-dev/blob/master/README.md) has to be changed?

@wkh237
Copy link
Owner

wkh237 commented Aug 13, 2017

Yeah, you're right, it should be test-init.js 👍

@lll000111 lll000111 changed the title Error normalization Error normalization (ongoing, tracking) Aug 16, 2017
@lll000111
Copy link
Contributor Author

lll000111 commented Aug 17, 2017

Status as of PR #489

NOTE: fs functions only.

I will update the Wiki with these details when/if those updates end up in a release.

Goal is the schema supported by react-native - see the Android Java implementation of the reject method (the iOS version does the same): The error object will have a code property in addition to the usual ones. The default code if none is sent by the native code module is "EUNSPECIFIED".

In addition to the general "EUNSPECIFIED" the linked PR introduces the following codes, on both Android and iOS unless noted otherwise. If there is a code marked "Android/iOS only" it doesn't mean that the other platform doesn't raise an error, only that it won't have that code.

The listed errors are not the only errors those functions return, of course, everything else is just "EUNSPECIFIED".

I use the ancient standard of Unix error codes whenever there is an appropriate one.

All functions also have "EINVAL" error codes when a mandatory parameter is missing, and those are TypeError instances, all others are Error.

fs.createFile

  • EEXIST (file already exists)

fs.writeFile, fs.appendFile

  • EISDIR (file is a directory)
  • ENOENT (does not exist AND failed to create)
  • ENOTDIR (Parent directory does not exist and could not be created)

fs.readFile

  • EISDIR
  • ENOENT

fs.readStream ("error" events)

  • ENOENT
  • EINVAL (unrecognized encoding - ANDROID ONLY)

fs.writeStream

  • ENOENT (does not exist AND failed to create)
  • EISDIR
  • ENOTDIR (Parent directory does not exist and could not be created)

fs.hash

  • EISDIR
  • ENOENT
  • EINVAL (wrong hash parameter)

fs.mkdir

  • EEXIST

fs.ls

  • ENOENT
  • ENODIR (file is not a directory)

fs.slice

  • EISDIR
  • ENOENT
  • EINVAL (URI could not be resolved)

Functions not mentioned here have nevertheless been updated, but they don't (yet) follow the scheme to provide a code property and still only provide text (which is different on iOS and Android). For example, unlink now throws errors in cases where it ignored them previously and still reported success.

@lll000111 lll000111 changed the title Error normalization (ongoing, tracking) Error normalization (ongoing, tracking issue) Aug 21, 2017
wkh237 pushed a commit that referenced this issue Aug 31, 2017
* fs.js: forgot one more error refactoring

* Fix path argument in iOS excludeFromBackupKey (#473)

* Fix link to fs.readStream() and to fs.writeStream() and insert link to new function fs.hash()

* Fix the documentation part of #467 "Example code for writeStream ignores that stream.write() returns a promise?"

* More fixes for issue #460 "Error normalization"

IMPORTANT: I wrote the iOS code BLIND (not even syntax highlighting) - this needs to be tested.

- Two or three methods that used callbacks to return results were changed to RN promises
- All methods using promises now use a Unix like code string for the first parameter, e.g. "ENOENT" for "File does not exist" (http://www.alorelang.org/doc/errno.html). The React Native bridge code itself uses this schema: it inserts "EUNSPECIFIED" when the "code" it gets back from Android/iOS code is undefined (null, nil). The RN bridge assigns the code (or "EUNSPECIFIED") to the "code" property of the error object it returns to Javascript, following the node.js example (the "code" property is not part of "standard" Javascript Error objects)
- Important errors like "No such file" are reported (instead of a general error), using the code property.
- I added a few extra error checks that the IDE suggested, mostly for Android (for which I have an IDE), if it seemed important I tried to do the same for teh iOS equivalent function
- I followed IDE suggestions on some of the Java code, like making fields private
- RNFetchBlobFS.java removeSession(): Added reporting of all failures to delete - IS THIS DESIRABLE (or do we not care)?
- readStream: The same schema is used for the emitted events when they are error events
- iOS: added an import for the crypto-digest headers - they are needed for the hash() function submitted in an earlier commit
- Fixed a link in the README.md - unfortunately the anchor-links change whenever even one character of the linked headline in the Wiki page changes

* Fix one issue raised in #477 by using code from https://stackoverflow.com/a/40874952/544779

* fix some access rights, remove unused items

* update gradle version setting in build.gradle

* Revert gradle settings to previous values :-(

* add a missing closing ")"

* Removed the part of an obsolete callback function parameter that I had left in when I converted mkdir to promises (low-level code)

* let mkdir resolve with "undefined" instead of "null" (my mistake)

* mkdir: normalize iOS and Android error if something already exists (file OR folder); return "true" (boolean) on success (failure is rejected promise) - it is not possibel to return "undefined" from a React Native promise from Java

* fix a long/int issue

* my mistake - according to https://facebook.github.io/react-native/docs/native-modules-android.html#argument-types "long" is not possible as argument type of an exported RN native module function

* Adde "utf8" as default encoding for fs.readFile - fixes #450 and #484

* follow my IDEA IDE's recommendations - SparseArray instead of HashMap, and make some fields private

* polyfill/File.js: add a parameter===undefined? check (this happened silently in the test suite)

* make var static again

* Normalized errors for fs.ls()

* forgot one parameter

* more parameter checks

* forgot to resolve the promise

* Forgot ;

* add more error parameter checks

* change readStream()/writeStream() default encoding to utf8 to match the tests in react-native-fetch-blob-dev

* default encoding is set in fs.js (now), no need to do it twice

* ReadStream error events: Set a default error code "EUNSPECIFIED" if no code is returned (should not happen, actually)

* writeFile() Android and iOS: improve errors; ReadStream: Add "ENOENT" (no such file) error event to Android version and add the thus far missing "code" parameter to iOS version

* oops - one "}" too many - removed

* add EISDIR error to readFile()s error vocabulary (iOS and Android)

* "or directory" is misplaced in a "no such file" error message for readFile()

* Android: two reject() calls did not have a code, iOS: slice() did not have code EISDIR, and "could not resolve URI" now is EINVAL everywhere

* writeStream: return ENOENT, EISDIR and EUNSPECIFIED according to the normalized schema (#460); Open a new question about behavior on ENOENT (#491)

* "+ +" was one plus sign too many

* this if has a whole block (that ois why I prefer a style where {} are mandatory even for single-statement blocks)

* I renamed this variable

* 1) #491 "writeStream() does not create file if it doesn't exist?"
2) I had gone overboard with the "@[..]" in the ios code, making some error strings arrays
3) fix typos: rename all ENODIR => ENOTDIR

* Java: getParentFolder() may return null - prevent a NullPointerException by adding one more check

* Relating to #298 -- looping through an array is not supposed to be done with for...in

*  Fix IOS syntax errors in #489

* #489 Fix typo and missing return statement

* fix error code
danielsuo added a commit to danielsuo/react-native-fetch-blob that referenced this issue Feb 13, 2018
* upstream/0.10.9:
  Fixed problem with type casting (wkh237#513)
  My proposed 0.10.9 changes (wkh237#489)
  wkh237#268 Cancelled task should not trigger `then` promise function
  Add ability to cancel android DownloadManager fetches (wkh237#502)
  Fix iOS initialization race condition (wkh237#499)
  prevent UIApplication methods from being called on background thread (wkh237#486)
  Implemenet fs.hash() -- wkh237#439 "Feature: Calculate file hash" (wkh237#476)
  I forgot one error string for the Android readFile() method (wkh237#475)
  Fix for wkh237#467 (wkh237#472)
  Fix for issue wkh237#468, wkh237#461, wkh237#460 and minor cleanup (wkh237#469)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants