Giter Site home page Giter Site logo

restful's People

Contributors

czukowski avatar dohnal avatar drahak avatar jirinapravnik avatar klimesf avatar kratkyzobak avatar kuzmamar avatar lucien144 avatar marten-cz avatar martinmystikjonas avatar mishak87 avatar pavoleichler avatar pupitooo avatar romanmatyus avatar s4muel avatar salamek avatar schneidermichal avatar stene3 avatar

Watchers

avatar

restful's Issues

XmlMapper: stringify appends new data for every call

Multiple calls of stringify method on the same data outputs different output. Is appends new data for every call.

echo $xmlMapper->stringify(array("a" => "b"));
echo $xmlMapper->stringify(array("a" => "b"));
<!-- first call -->
<?xml version="1.0" encoding="UTF-8"?>
<root>
  <a>b</a>
</root>


<!-- second call -->
<?xml version="1.0" encoding="UTF-8"?>
<root>
  <a>b</a>
</root>
<root>
  <a>b</a>
</root>

Application\Responses\ErrorResponse

Another suggestion we would find useful is to create new class Application\Responses\ErrorResponse inherited from TextResponse which will represent be used for returning errors.

It will allows simplify testing if returned response is error or not.

What do you think? Should i prepare PR?

Add support for custom error messages in sendErrorResponse

It would be nice to have some possibility to change error message of "system errors". For example invalid Accept header may cause error with message Unregistered API response. which doesn't say too much to client.

Method sendErrorResource can have second argument with optional message overwriting the original in exception. With this, I could define my own sendErrorResource somehow like this:

protected function sendErrorResource(\Exception $e, $message = NULL) {
    $code = $e->getCode();
    if(isset($this->httpCodeErrorMessages[$code])){
        $message = $this->httpCodeErrorMessages[$code]; 
    }else{
        $message = $e->getMessage();
    }

    parent::sendErrorResource($e, $message);
}

XmlMapper: ignoring attributes depends on existance of value of element

I found strange behaviour of XmlMapper. This xml:

<root>
    <user attr="test1" />
    <user attr="test2">value</user>
</root>

is converted to:

array (1)
   user => array (2)
   |  0 => array (1)
   |  |  "@attributes" => array (1)
   |  |  |  attr => "test1"
   |  1 => "value"

In other words, if I specify attributes, they are ignored only if there is some value. I think attributes should be always ignored (or parsed, but I don't like it so much).

Testing resource presenters

Is there any best practice for testing ResourcePresenters? We start using your library (which is by far best we can found), but we struggle in creation of Input objects, because it seems hard-coded to use HttpRequest.

So my question is: how to you test ResourcePresenter? It there some practice I missed?

If not I will prepare PR with changes which allows to do so (basically just customizable InputFactory which allows to make explicit request)

XmlMapper: correct creating of array in output (remove item elements)

I think that following code should return the same output as the recieved input:

$this->resource = $this->resourceFactory->create($this->input->getData());

It works for JSON, but not for XML:

Input:

<root> 
    <user>
        <name>John</name>
    </user>
    <user>
        <name>Jack</name>
    </user>
</root>

Output:

<root>
    <user>
        <item index="0">
            <name>John</name>
        </item>
        <item index="1">
            <name>Jack</name>
        </item>
    </user>
</root>

I understand the <root> hardcoding, but the <item> can be removed, doesn't it?

Support for Hypermedia API

It would be usefull to have Hypermedia API support, for example in "application/HAL+json" format. Do you plan this feature ?

XmlMapper: parsed input is not recursively converted to array

If I dump result from $this->input->getData() in ResourcePresenter for following xml input, it returns array of SimpleXmlElements. Shouldn't it return array of arrays (recursively converted)? For simmilar json input it is recursively converted to stdClasses.

<?xml version="1.0" encoding="UTF-8"?>
<envelope>
   <user>
     <name>test</name>
     <phone>500</phone>
  </user>
</envelope>

Actually, implementation is very easy. In XmlMapper::fromXml may be used this solution

Disable GET method overriding

According to HTTP standard, GET method should never change data on the server. Currently I'm able to override GET HTTP method to behave like POST by X-HTTP-Method-Override header or even by __method query parameter which is not correct. Overridding should be possible only for POST method. See this article for example.

Will you implement this limitation to library or is there any easy way how to do it easily in my application?

Add support for validation of deep input structures

I really like the input data validation, but one big problem of current implementation is that you cannot validate deeper structures. E.g.

<?xml version="1.0" encoding="UTF-8"?>
<envelope>
   <user>
     <name>test</name> <!-- how to validate this value? -->
     <phone>500</phone>
  </user>
  <user>
     <name>test2</name>
     <phone>400</phone>
  </user>
</envelope>

I think there should be some way to do it. I have tried to implement it by defining own ValidationScope and it was pretty easy. See following solution:

class ValidationScope extends \Drahak\Restful\Validation\ValidationScope {

    /**
     * Validate all fields in collection deeply
     * @param array $data
     * @return \Drahak\Restful\Validation\Error[]
     */
    public function validate(array $data) {

        $errors = array();

        foreach ($this->fields as $field) {
            $fieldErrors = $this->validateDeeply($field, $field->getName(), $data);

            if (!$fieldErrors) {
                continue;
            }
            $errors += $fieldErrors;
        }
        return $errors;
    }

    /**
     * Validate field against given data structure
     * - e.g. "user.address.street" validates $data["user"]["address"]["street"]
     * All items in lists are validated
     * - e.g. for more users, $data["user"][0]["address"]["street"] and $data["user"][1]["address"]["street"] is validated
     * @param \Drahak\Restful\Validation\Field $field
     * @param string $path Key path to be checked in $data, the nesting separator is dot
     * @param array $data Deep structure to be validated
     * @return \Drahak\Restful\Validation\Error[]
     */
    private function validateDeeply(\Drahak\Restful\Validation\Field $field, $path, $data){

        $errors = array();

        if(\Nette\Utils\Arrays::isList($data)){ // check every item of list
            foreach($data as $item){
                $newErrors = $this->validateDeeply($field, $path, $item);
                $errors = array_merge($errors, $newErrors);
            }
        }else{
            $keys = explode(".", $path);
            foreach($keys as $key){
                $newPath = \Nette\Utils\Strings::replace($path, "~^$key\.~");

                if(isset($data[$key])){
                    if(is_array($data[$key])){
                        $newErrors = $this->validateDeeply($field, $newPath, $data[$key]);
                        $errors = array_merge($errors, $newErrors);
                    }else{
                        $newErrors = $field->validate($data[$key]);
                        $errors = array_merge($errors, $newErrors);
                    }
                }else{
                    // value is not present
                }
            }
        }

        return $errors;
    }

But I had to copy-paste also field method and $fields property, because this field is private in library (not protected). I think that this should be changed too (maybe you can check other places in library which could be extended and contains such private properties).

With this solution you can easily check the example above by:

$this->input->field('user.name')
            ->addRule(IValidator::MIN_LENGTH, "Min 10 chars", 10);

The "problem" of this solution is that you cannot determine which exact node of structure caused the validation error (from the error reponse). So what do you think?

Custom error codes

When displaying errors to clients, the current format is:

{
    "code": 404,
    "status": "error",
    "message": "Item not found."
}

If the code of the exception ($exc->getCode()) falls in the range of HTTP status codes, it is used both in the HTTP response header and the response body ("code": 404). For all other exception codes, both the HTTP response status code and the code in the response body will be 400.

This is great for simple tasks, however one may need to provide the client with a more detailed error status codes than the ones defined in the HTTP spec. Consider the following:

/books/157?user_id=12&access_token=XXX
The request may fail because the user of ID 12 does not exist, the user 12 is not allowed to see this resource, the access_token is invalid or the book of this ID does not exist, etc.
The client app may need to act differently based on the exact error encountered. However having only the HTTP status codes available, it may lack a unique machine-readable identifier to act upon.

May I propose to either leave the "code" in the response body to always match the $exc->getCode():

throw new Exception('My error.', 12345); // will send a HTTP 400 containing {"code": 12345 ... }

Or maybe addind a way to modify data in the response generated by the ResourcePresenter::sendErrorResource() method.
This may be as simple as providing a ResourcePresenter::createErrorResource($exc) method which converts the exception to the resource and could be overriden easily by extendning the ResourcePresenter.
Or by providing an ErrorResourceFactory which would take care of the exception to resource conversion and would be easy to replace by a custom implementation.

In case you are willing to accept any of the modifications proposed, I would be happy to prepare a pull request.

XmlMapper: check for invalid tag names?

In XML, not every character can be used for tag name. For example spaces are not allowed. Maybe there should be some validation for that. You can easily forget this limitation. In JSON, there is no such problem.

Or do you want to let DomDocument throw its exception? See this post.

Unable to disable jsonp support

Documentation says that disabling this functionality can be done by jsonpKey: FALSE in config, but this option causes exception thrown by validation in RestfulExtension:

Validators::assert($config['jsonpKey'], 'string');

Cannot mock AuthenticationProcess

Another issue we encounter while writing tests. In class AuthenticationProcess is method autheticate final. Therefore cannot be intercept when we mocking this class in tests.

I suggest to remove final modifier here if there is not any serious reason to keep it. Do you agree?

Using PHP-5.4 construct

You are specifying PHP-5.3 as a dependency for this library. But you are using PHP-5.4 construct in src/Drahak/Restful/Input.php:

return $isset ? $isset : isset($this->getData()[$name]);

Function array dereferencing is available only in PHP-5.4+

$foo = foo()[0];

Add shortcut for setting up Rule's code

Currently I've to do something like this to setup a code to field's rule:

$this->input->field('user')
        ->addRule(IValidator::MIN_LENGTH, "Min 10 chars", 10);

$rules = $this->input->field('user')->getRules();
$lastRule = end($rules);
$lastRule->setCode(22);

It would be nice to have some shortcut for that. Something like:

$this->input->field('user')
        ->addRule(IValidator::MIN_LENGTH, "Min 10 chars", 10, 22);

I consider error codes very important when implementing REST API.

MethodHandler: insufficient recognition for 405 error

I get an 405 error for passing non-numeric query parameter $test to following presenter's action:

public function actionDefault($test = 1) {}

Nette throws correct 404 BadRequestException with message:
Invalid value for parameter 'test' in method MyPresenter::actionDefault(), expected integer.

Route is defined in the easiest possible way:

new ResourceRoute('/api/', 'My:default', IResourceRouter::GET);

ResponseProxy: use __call method for full compatibilty with Response

Restful\Http\ResponseProxy class should use __call method to provide full compatibility with Nette\Http\Response.

For exampe when using this Nette method (nette/http#9), it tries to call static method date on an instance of Nette\Http\Response, which fails when using ResponseProxy.

Suggested method:

public function __call($name, $args) {
        if ($this->response->reflection->hasMethod($name)) {
            return call_user_func_array(array($this->response, $name), $args);
        } else {
            return parent::__call($name, $args);
        }
}

With this magic, you can also remove all methods which are just re-calling original Nette\Http\Response class. You cannot due to IResponse interface implementation.

MappingException cannot be easily caught and causes server error

If I pass invalid Content-type header or just invalid content (e.g. invalid xml or json), exception is thrown in InputFactory. But this happens very early because Input class is created from DIC and it cannot be easily catched. This leads to hard 500 error.

It can be fixed by creating Input on demand in ResourcePresenter (all occurences of $this->input must be replaced with $this->getInput() and this property should become private). Also, InputFactory must be injected to presenter instead of Input class.

public function getInput(){
    if(!$this->input){
        try {
            $this->input = $this->inputFactory->create();
        } catch(\Nette\Application\BadRequestException $e){
            $this->sendErrorResource($e);
        }
    }

    return $this->input;
}

Small fix is also needed in InputFactory, because mapper parsing may fail as well as getting correct mapper, so something like this is needed:

try {
    $requestBody = $this->mapper->parse($input);
} catch (\Drahak\Restful\Mapping\MappingException $e) {
    throw new BadRequestException($e->getMessage(), 400, $e);
}

Missing validation rule: callback

Is it possible to add callback validation of input fields?

Possible usage:

$this->input->field("user")
            ->addRule(IValidator::CALLBACK, "message", array($this, "callback"));

or

$this->input->field("user")
            ->addRule(array($this, "callback"), "message", $args);

Setter for private key

Is it possible to implement setter for private key in Drahak\Restful\Security\Authentication\HashAuthenticator, because I'd like to use different key for each user of my API?

Stable release

Is there going to be a stable release? Is there any checklist needed to be done? I'd be glad to help, this is an awesome library!

Flip order of validation of input data with validation of supported media types

In current implementation, with invalid accept header (with fixed #16) you get an validation error in fallback format (because thats probably the only solution of issue #16) and after you fix the validation you get an error about unsupported media type. I think this should be checked in opposite direction.

Do you think you will add this to lib? If not, there should be at least some getter for registred reponses in Application\ResponseFactory to resolve this manually in checkReuirements method.

P.S. Possible fix for mentioned issue:

protected function sendErrorResource(\Exception $e) {
  // ... remains the same
  try {
      $response = $this->responseFactory->create($this->resource);
      $this->sendResponse($response);

  } catch (InvalidStateException $e) {

      $this->resource->setContentType(IResource::JSON); // fallback
      $response = $this->responseFactory->create($this->resource);
      $this->sendResponse($response);
  }
}

Missing validation rule: required

Is there any reason not to have some REQUIRED validation rule?

I understand that in current implementation, every field with some rule (and without OPTIONAL rule) is somehow required, but It would be nice to define required fields like this

$this->input->field('user')
            ->addRule(IValidator::REQUIRED, "Required!");

except like this

$this->input->field('user')
            ->addRule(IValidator::MIN_LENGTH, "Required!", 1);

InputFactory: use HttpRequest::getRawBody in Nette 2.2

In Nette 2.2, there is a method to retrieve raw body of HTTP request (finally). InputFactory should use it if it exists. It allows much easier testing.

if(ClassType::from($this->httpRequest)->hasMethod("getRawBody")) {
    $input = $this->httpRequest->getRawBody();
} else {
    $input = file_get_contents('php://input');
}

Insufficient creation of HttpRequest in Nette 2.3

Due to this commit, getRawBody() will always return NULL, because in ApiRequestFactory there is no callback passed. Possible quickfix:

    public function createHttpRequest()
    {
        $request = $this->factory->createHttpRequest();
        $url = $request->getUrl();
        $url->setQuery($request->getQuery());

        $rawBodyCallback = function() use ($request) {
            return $request->getRawBody();
        };

        return new Request(
            $url, NULL, $request->getPost(), $request->getFiles(), $request->getCookies(), $request->getHeaders(),
            $this->getPreferredMethod($request), $request->getRemoteAddress(), $request->getRemoteHost(), $rawBodyCallback
        );
    }

Correct creating of ResourceRoutes

I'm a bit confused about creating routes, mostly about the $flags param. If I define route like this

new ResourceRoute('/api/', "Homepage:default", IResourceRouter::GET);

and try to access this resource (the URL) with POST method, I get an 404 error because it simple doesn't match. But thats wrong, I would expect to return 405 error (because the resource exists!).

For that behaviour I have to define the route like this:

new ResourceRoute('/api/', array(
    'presenter' => 'Homepage',
    'action' => array(
        IResourceRouter::GET => 'default'
    )
), IResourceRouter::RESTFUL);

If I understand this correctly, the last parameter should always be RESTFUL to get correct 405 errors (and the simple definition of action and presenter should be deprecated - I mean the first example). Am I right?

Returning 404 Not found is not possible

How can I send 404 from actionRead in case resource with given Id does not exists?

Currently I thow BadRequestException with 404 code. But it does not works. Respons returned has error message "Method not supported".

Reason is probably this piece of code in MethodHandler:

public function error(Application $application, \Exception $e)
    {
        if ($e instanceof NetteBadRequestException && $e->getCode() === 404) {
            $this->checkAllowedMethods();
        }
    }

Is it error or should i return 404 in different way?

sendErrorReponse forces JSON output

In documentation of Error presenter and sendErrorReponse method you say: Clients can determine preferred format just like in normal API resource. Actually it only adds data from exception to resource and send it to output.

But in code, there is hardcoded JSON output. Is this line just forgotten or does It have some reason?

Dynamic private key

Would by great if the class Drahak\Restful\Security\Process\SecuredAuthentication has setter for private key or something else how to dynamically change private key. I implemented this my own, but Im not sure is it right. Setter in SecuredAuthentication, setter in HashAuthenticator but there is also an interface for TimeoutAuthenticator so i had to remove it.

Return allow header with 405 HTTP code

There is a standard HTTP header (Allow) for letting client know about available methods which can be used. Currently the 405 error return possible methods only in response body.

Arrays::isList is not compatible with 2.0.*

Method ValidationScope::validateDeeply fails on older versions of Nette (2.0.*), because Arrays::isList is not implemented. Is it possible to call Validators::isList instead?

Gzip: add support for processing compressed requests

Gzip compressing is very popular and useful in APIs, it would be nice to support it. It is actually very easy to implement. Just add following lines to InputFactory::parseRequestBody before mapper parsing:

if($this->httpRequest->getHeader('Content-encoding') === "gzip")){ // or contains?
    $input = gzdecode($input);
}

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.