Skip to content

Add Base Exception class#218

Merged
abnegate merged 26 commits into
utopia-php:feat-base-exceptionfrom
everly-gif:add-exception-class
Apr 28, 2023
Merged

Add Base Exception class#218
abnegate merged 26 commits into
utopia-php:feat-base-exceptionfrom
everly-gif:add-exception-class

Conversation

@everly-gif

Copy link
Copy Markdown
Contributor

Following the changes in this PR, we need to add some base Database Exception that we could use to distinguish between our errors and native errors. This PR implements a base exception class.

@everly-gif everly-gif marked this pull request as draft November 1, 2022 19:23

@christyjacob4 christyjacob4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good 👌

Can you also scan through the code base and check if there are usages of exceptions other than Utopia\Database\Exception or Utopia\Database\Exception\Authorization etc ?

Basically we want to ensure that every custom error thrown by the database library is an instance of the Database library exception or its sub classes.

@everly-gif

Copy link
Copy Markdown
Contributor Author

Looks good 👌

Can you also scan through the code base and check if there are usages of exceptions other than Utopia\Database\Exception or Utopia\Database\Exception\Authorization etc ?

Basically we want to ensure that every custom error thrown by the database library is an instance of the Database library exception or its sub classes.

I have updated every instance where we throw an exception to be a type of Utopia\Database\Exception. But there are still certain uses of \Exception such as places below

  try  {
            new \DateTime($datetime);
        }
        catch(\Exception $e) {
            return false;
        }

ie. in cases where we don't throw an exception explicitly.

@everly-gif everly-gif marked this pull request as ready for review November 4, 2022 17:20
Comment thread src/Database/Adapter/MariaDB.php Outdated
Comment thread src/Database/Adapter/MariaDB.php Outdated
Comment thread src/Database/Adapter/MariaDB.php Outdated
Comment thread src/Database/Adapter/MariaDB.php Outdated
Comment thread src/Database/Adapter/Mongo.php Outdated
use Swoole\Coroutine\Client as CoroutineClient;
use Utopia\Database\Adapter\Mongo\Auth;
use Utopia\Database\Adapter\Mongo\MongoClientOptions;
use Utopia\Database\Document;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you sync your branch with the base branch ? I don't think this file is expected to be here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am on the latest version of main

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed this file

Comment thread src/Database/Adapter/MySQL.php Outdated
Comment thread src/Database/Adapter/SQLite.php Outdated
Comment thread src/Database/Adapter/SQLite.php Outdated
Comment thread src/Database/Adapter/SQLite.php Outdated
Comment thread src/Database/Adapter/SQLite.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Database.php Outdated
Comment thread src/Database/Validator/Roles.php Outdated
try {
$role = Role::parse($role);
} catch (\Exception $e) {
} catch (Exception $e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm.. is this necessary ? Do we need to explicitly import the native exception class ?

Comment thread src/Database/Role.php Outdated
Comment thread src/Database/Query.php Outdated
// Check for deprecated query syntax
if (\str_contains($method, '.')) {
throw new \Exception("Invalid query method");
throw new DatabaseException("Invalid query method");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay after reviewing Role, Query and Permission I don't think we need to expose any of those errors in the UI either. Let's revert these files back too

Comment thread src/Database/Adapter/MariaDB.php Outdated
break;
default:
throw new Exception('Unknown Type');
throw new DatabaseException('Unknown Type. Must be one of ${Database::VAR_INTEGER}, ${Database::VAR_FLOAT}, ${Database::VAR_BOOLEAN}, ${Database::VAR_DOCUMENT}, ${Database::VAR_DATETIME}');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new DatabaseException('Unknown Type. Must be one of ${Database::VAR_INTEGER}, ${Database::VAR_FLOAT}, ${Database::VAR_BOOLEAN}, ${Database::VAR_DOCUMENT}, ${Database::VAR_DATETIME}');
throw new DatabaseException("Unknown Type. Must be one of ${Database::VAR_INTEGER}, ${Database::VAR_FLOAT}, ${Database::VAR_BOOLEAN}, ${Database::VAR_DOCUMENT}, ${Database::VAR_DATETIME}");

String interpolation only works when the string is enclosed in double quotes

Comment thread src/Database/Adapter/Mongo.php Outdated

default:
throw new Exception('Unknown Operator:' . $operator);
throw new DatabaseException('Unknown Operator:' . $operator .'. Must be one of ${Query::TYPE_EQUAL}, ${Query::NOTEQUAL, ${Query::TYPE_LESSER}, ${Query::TYPE_LESSEREQUAL}, ${Query::TYPE_GREATER}, ${Query::TYPE_GREATEREQUAL}, ${Query::TYPE_CONTAINS}, {Query::TYPE_SEARCH}');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new DatabaseException('Unknown Operator:' . $operator .'. Must be one of ${Query::TYPE_EQUAL}, ${Query::NOTEQUAL, ${Query::TYPE_LESSER}, ${Query::TYPE_LESSEREQUAL}, ${Query::TYPE_GREATER}, ${Query::TYPE_GREATEREQUAL}, ${Query::TYPE_CONTAINS}, {Query::TYPE_SEARCH}');
throw new DatabaseException('Unknown Operator:' . $operator .". Must be one of ${Query::TYPE_EQUAL}, ${Query::NOTEQUAL}, ${Query::TYPE_LESSER}, ${Query::TYPE_LESSEREQUAL}, ${Query::TYPE_GREATER}, ${Query::TYPE_GREATEREQUAL}, ${Query::TYPE_CONTAINS}, {Query::TYPE_SEARCH}");

@abnegate abnegate changed the base branch from main to feat-base-exception April 28, 2023 05:54
@abnegate abnegate deleted the branch utopia-php:feat-base-exception April 28, 2023 05:56
@abnegate abnegate closed this Apr 28, 2023
@abnegate abnegate reopened this Apr 28, 2023
@abnegate abnegate merged commit f7d8ae6 into utopia-php:feat-base-exception Apr 28, 2023
@abnegate abnegate mentioned this pull request Apr 28, 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.

5 participants