Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Json format: refactor error handling + add JSON_UNESCAPED_UNICODE #25

Merged
merged 4 commits into from
Sep 3, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 43 additions & 14 deletions src/Database.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
use Filebase\Filesystem;
use Filebase\Document;
use Filebase\Backup;
use Filebase\Format\DecodingException;
use Filebase\Format\EncodingException;
use Filebase\Filesystem\SavingException;
use Filebase\Filesystem\ReadingException;
use Filebase\Filesystem\FilesystemException;

class Database
{
Expand All @@ -31,9 +36,12 @@ class Database


/**
* __construct
*
*/
* Database constructor.
*
* @param array $config
*
* @throws FilesystemException
*/
public function __construct(array $config = [])
{
$this->config = new Config($config);
Expand All @@ -46,12 +54,12 @@ public function __construct(array $config = [])
{
if (!@mkdir($this->config->dir, 0777, true))
{
throw new Exception(sprintf('`%s` doesn\'t exist and can\'t be created.', $this->config->dir));
throw new FilesystemException(sprintf('`%s` doesn\'t exist and can\'t be created.', $this->config->dir));
}
}
else if (!is_writable($this->config->dir))
{
throw new Exception(sprintf('`%s` is not writable.', $this->config->dir));
throw new FilesystemException(sprintf('`%s` is not writable.', $this->config->dir));
}
}

Expand Down Expand Up @@ -240,13 +248,14 @@ public function count()
* @param $document \Filebase\Document object
* @param mixed $data should be an array, new data to replace all existing data within
*
* @throws Exception|SavingException
* @return (bool) true or false if file was saved
*/
public function save(Document $document, $wdata = '')
{
if ($this->config->read_only === true)
{
throw new Exception("This database is set to be read-only. No modifications can be made.");
throw new SavingException("This database is set to be read-only. No modifications can be made.");
}

$format = $this->config->format;
Expand All @@ -270,7 +279,12 @@ public function save(Document $document, $wdata = '')

$document->setUpdatedAt(time());

$data = $format::encode( $document->saveAs(), $this->config->pretty );
try {
$data = $format::encode( $document->saveAs(), $this->config->pretty );
} catch (EncodingException $e) {
// TODO: add logging
throw new SavingException("Can not encode document.", 0, $e);
}

if (Filesystem::write($file_location, $data))
{
Expand Down Expand Up @@ -302,17 +316,32 @@ public function query()
//--------------------------------------------------------------------



/**
* read
*
* @param string $name
* @return decoded file data
*/
* @param $name
* @return bool
* @throws Exception|ReadingException
*/
protected function read($name)
{
$format = $this->config->format;
return $format::decode( Filesystem::read( $this->config->dir.'/'.Filesystem::validateName($name, $this->config->safe_filename).'.'.$format::getFileExtension() ) );

$file = Filesystem::read(
$this->config->dir . '/'
. Filesystem::validateName($name, $this->config->safe_filename)
. '.' . $format::getFileExtension()
);

if (!$file) {
/**
* FIXME: shouldn't we raise an exception in this case? + implement add/create method?
Copy link
Author

Choose a reason for hiding this comment

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

not sure if its ok from users viewpoint that get (read) method returns empty document object if file doesn't exists? seems weird
maybe we should throw en exception in get (read) and implement add/create, getOrCreate or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

I think the initial idea was to provide production sites with safety in case of failure so that you could create new docs without checking if old existed. That's why I built the method has() which checks if the document exists.

An issue like this came up before where things created when I didn't want them too, so there is a config option for read_only so that it only reads the documents that exist. (of course thats not good if you do want to make updates, so its a narrow use-case. But yeah it's not perfect, and could use a bit more transparency for the developer.

Copy link
Member

Choose a reason for hiding this comment

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

But as to your point, the get() was designed to get a document (from the file system) or an empty document if doenst exist. But only creates a new document in the filesystem if you call the save() method. This way you can get all the docs you want and then populate data, so if it exists or not intially, it can be created or updated.

Copy link
Member

Choose a reason for hiding this comment

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

@assemblie Do you have a way to chat/talk. Like email, or slack etc? This way we can discuss without having to comment each time? really need a github messaging system...

Copy link
Author

Choose a reason for hiding this comment

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

@timothymarois ok i get it imo this approach is ok but we need to state this in documentation explicitly and yes if you have slack workspace invite me here [email protected]

Copy link
Member

Choose a reason for hiding this comment

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

@assemblie Agreed. and invite sent.

* or use $this->has (maybe not here but inside $this->get method) to verify that document exists
* if not than use add/create method?
* or remove this exception and allow creating new document with this method?
*/
// throw new ReadingException("Document '{$name}' does not exists.");
}

return $format::decode($file);
}


Expand Down
6 changes: 6 additions & 0 deletions src/Filesystem/FilesystemException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php namespace Filebase\Filesystem;

class FilesystemException extends \Exception
{

}
6 changes: 6 additions & 0 deletions src/Filesystem/ReadingException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php namespace Filebase\Filesystem;

class ReadingException extends FilesystemException
{

}
6 changes: 6 additions & 0 deletions src/Filesystem/SavingException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php namespace Filebase\Filesystem;

class SavingException extends FilesystemException
{

}
6 changes: 6 additions & 0 deletions src/Format/DecodingException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php namespace Filebase\Format;

class DecodingException extends FormatException
{

}
6 changes: 6 additions & 0 deletions src/Format/EncodingException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php namespace Filebase\Format;

class EncodingException extends FormatException
{

}
18 changes: 18 additions & 0 deletions src/Format/FormatException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php namespace Filebase\Format;

class FormatException extends \Exception
{
private $inputData;

public function __construct($message, $code = 0, \Exception $previous = null, $inputData = null)
{
parent::__construct($message, $code, $previous);
$this->inputData = $inputData;
}

public function getInputData()
{
return $this->inputData;
}
}

54 changes: 39 additions & 15 deletions src/Format/Json.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@

class Json implements FormatInterface
{

/**
* getFileExtension
*
*/
* @return string
*/
public static function getFileExtension()
{
return 'json';
Expand All @@ -18,28 +16,54 @@ public static function getFileExtension()


/**
* encode
*
*/
* @param array $data
* @param bool $pretty
* @return string
* @throws FormatException
*/
public static function encode($data = [], $pretty = true)
{
$p = 1;
if ($pretty==true) $p = JSON_PRETTY_PRINT|JSON_UNESCAPED_SLASHES;

return json_encode($data, $p);
$options = 0;
if ($pretty == true) {
$options = JSON_PRETTY_PRINT|JSON_UNESCAPED_SLASHES|JSON_UNESCAPED_UNICODE;
}

$encoded = json_encode($data, $options);
if ($encoded === false) {
throw new EncodingException(
"json_encode: '" . json_last_error_msg() . "'",
0,
null,
$data
);
}

return $encoded;
}


//--------------------------------------------------------------------


/**
* decode
*
*/
* @param $data
* @return mixed
* @throws FormatException
*/
public static function decode($data)
{
return json_decode($data, 1);
$decoded = json_decode($data, true);

if ($data !== false && $decoded === null) {
throw new DecodingException(
"json_decode: '" . json_last_error_msg() . "'",
0,
null,
$data
);
}

return $decoded;
}


Expand Down
17 changes: 17 additions & 0 deletions tests/DatabaseTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php namespace Filebase;


use Filebase\Filesystem\SavingException;

class badformat {

}
Expand Down Expand Up @@ -241,4 +243,19 @@ public function testDatabaseFindAllDataOnly()
$db->flush(true);
}

public function testDatabaseSavingNotEncodableDocument()
{
$this->expectException(SavingException::class);

$db = new \Filebase\Database([
'dir' => __DIR__.'/databases'
]);

$doc = $db->get("testDatabaseSavingNotEncodableDocument");

// insert invalid utf-8 characters
$doc->testProp = "\xB1\x31";

$doc->save();
}
}