Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework based on #18 Normalize fields #25

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
7 changes: 3 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
language: php

php:
- '7.2'
- '7.3'

cache:
Expand All @@ -18,11 +17,11 @@ before_script:

after_script:
- echo $TRAVIS_BRANCH
- if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.2" ]]; then echo "Sending coverage report"; vendor/bin/test-reporter --coverage-report build/logs/clover.xml; fi
- if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.2" ]]; then echo "Sending codecov report"; TRAVIS_CMD="" bash <(curl -s https://codecov.io/bash) -f build/logs/clover.xml; fi
- if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.3" ]]; then echo "Sending coverage report"; vendor/bin/test-reporter --coverage-report build/logs/clover.xml; fi
- if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.3" ]]; then echo "Sending codecov report"; TRAVIS_CMD="" bash <(curl -s https://codecov.io/bash) -f build/logs/clover.xml; fi

script:
- if [[ ${TRAVIS_PHP_VERSION:0:3} != "7.2" ]]; then NC="--no-coverage"; fi
- if [[ ${TRAVIS_PHP_VERSION:0:3} != "7.3" ]]; then NC="--no-coverage"; fi
- ./vendor/phpunit/phpunit/phpunit $NC

notifications:
Expand Down
52 changes: 43 additions & 9 deletions src/Api.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace atk4\api;

use atk4\data\Field;
use atk4\data\Model;
use Laminas\Diactoros\Request;
use Laminas\Diactoros\Response\JsonResponse;
Expand Down Expand Up @@ -41,6 +42,13 @@ class Api
/** @var int Response options */
protected $response_options = JSON_PRETTY_PRINT | JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT;

/**
* @var bool If set to true, the first array element of Model->export
* will be returned (GET single record)
* If not, the array will be returned as-is
*/
public $single_record = true;

/**
* Reads everything off globals.
*
Expand Down Expand Up @@ -148,14 +156,29 @@ public function exec($callable, $vars = [])
// if callable function returns agile data model, then export it
// this is important for REST API implementation
if ($ret instanceof Model) {
$ret = $this->exportModel($ret);
$data = [];

$allowed_fields = $this->getAllowedFields($ret, 'read');
if ($this->single_record) {
$data = $this->exportModel($ret, $allowed_fields);
} else {
foreach ($ret as $m) {
$data[] = $this->exportModel($m, $allowed_fields);
}
}

$ret = $data;
}

// no response, just step out
if ($ret === null) {
return;
}

if ($ret === true) { // manage delete
$ret = [];
}

// emit successful response
$this->successResponse($ret);
}
Expand Down Expand Up @@ -185,15 +208,23 @@ protected function call($callable, $vars = [])
*
* Extend this method to implement your own field restrictions.
*
* @param Model $m
* @param Model $m Model
* @param array $allowed_fields Allowed fields
*
* @throws \atk4\data\Exception
* @throws \atk4\core\Exception
*
* @return array
*/
protected function exportModel(Model $m)
protected function exportModel(Model $m, array $allowed_fields = [])
{
return $m->export($this->getAllowedFields($m, 'read'));
$data = [];
foreach ($allowed_fields as $fieldName) {
$field = $m->getField($fieldName);
$data[$field->actual ?? $fieldName] = $field->toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think using actual is sensible. Think of this example Model:

$this->addField('name');
$join = $this->join('some_table');
$join->addField('other_name', ['actual' => 'name']);

That would lead to replacing the name of the model's name field with the name field from the joined table, which was sensibly declared in the model as other_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the demo there was a Model with few fields with actual defined and even if i don't use it i try to fix the right field name to return back in api response

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking of it this way:
A Model can have each field name only once. So no trouble using this as array key.
When using actual property, we can have duplicate array keys, which leads to false/incomplete data passed via API.
I think actual is meant for Persistences only.

}

return $data;
}

/**
Expand Down Expand Up @@ -241,7 +272,7 @@ protected function loadModelByValue(Model $m, $value)
protected function getAllowedFields(Model $m, $action = 'read')
{
// take model only_fields into account
$fields = is_array($m->only_fields) ? $m->only_fields : [];
$fields = is_array($m->only_fields) && !empty($m->only_fields) ? $m->only_fields : array_keys($m->getFields());

// limit by apiFields
if (isset($m->apiFields, $m->apiFields[$action])) {
Expand Down Expand Up @@ -296,7 +327,7 @@ protected function successResponse($response)
// for testing purposes there can be situations when emitter is disabled. then do nothing.
if ($this->emitter) {
$this->emitter->emit($this->response);
exit;
exit; // @todo find a solution to remove this exit.
}

// @todo Should we also stop script execution if no emitter is defined or just ignore that?
Expand Down Expand Up @@ -394,6 +425,7 @@ public function rest($pattern, $model = null, $methods = ['read', 'modify', 'del
// GET all records
if (in_array('read', $methods)) {
$f = function (...$params) use ($model) {
$this->single_record = false;
if (is_callable($model)) {
$model = $this->call($model, $params);
}
Expand All @@ -416,8 +448,9 @@ public function rest($pattern, $model = null, $methods = ['read', 'modify', 'del
$model->onlyFields($this->getAllowedFields($model, 'read'));

// load model and get field values
return $this->loadModelByValue($model, $id)->get();
return $this->loadModelByValue($model, $id);
};

$this->get($pattern.'/:id', $f);
}

Expand All @@ -437,7 +470,7 @@ public function rest($pattern, $model = null, $methods = ['read', 'modify', 'del
$this->loadModelByValue($model, $id)->save($this->request_data);
$model->onlyFields($this->getAllowedFields($model, 'read'));

return $model->get();
return $model;
};
$this->patch($pattern.'/:id', $f);
$this->post($pattern.'/:id', $f);
Expand All @@ -457,7 +490,8 @@ public function rest($pattern, $model = null, $methods = ['read', 'modify', 'del
$model->onlyFields($this->getAllowedFields($model, 'read'));

$this->response_code = 201; // http code for created
return $model->get();

return $model;
};
$this->post($pattern, $f);
}
Expand Down
Loading