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

Detect correct type on request when there are multiple media types mapped to the same content type #1335

Open
wants to merge 7 commits into
base: 1.2
Choose a base branch
from

Conversation

gscherer
Copy link
Contributor

@gscherer gscherer commented Jul 19, 2018

In the case where multiple types are registered to lithium\net\http\Media with the same content type, even when distinguished by conditions, an instance of lithium\action\Request will fail to determine the correct type to use to decode the request body. For example, if we register another type which uses 'application/json' as a content type like this:

use lithium\net\http\Media;

Media::type('json_base64', ['application/json'], [
    'encode' => function ($data) {
        return base64_encode(json_encode($data));
    },
    'decode' => function ($data) {
        return json_decode(base64_decode($data), true);
    },
    'cast' => true,
    'conditions' => [
        'http:content_transfer_encoding' => 'base64'
    ]
]);

A call to Media::type('application/json') will return an array like:

[
	'json_base64',
	'json'
]

This case is not handled by lithium\net\http\Message::type(), which causes lithium\action\Request::$_type to be assigned as the content type of 'application/json'.

Futhermore, when Request::body() is called to decode the request body, the Media class fails to find a handler for 'application/json' (it expects a short name like 'json').

To fix this, lithium\net\http\Message::type() must be extended by lithium\action\Request. In the case that $type is a string and equals the result of the parent's call, we can loop over each type returned by Media::type() and attempt to match the request to it like this:

public function type($type = null) {                                            
    if (!$type && !empty($this->params['type'])) {                              
        $type = $this->params['type'];                                          
    }                                                                           
    $_type = parent::type($type);                                               
    if (is_string($type) && $_type === $type) {                                 
        $media = $this->_classes['media'];                                      
        $content = $media::type($type);                                         
        if (is_array($content) && !isset($content['content'])) {                
            foreach ($content as $short_type) {                                 
                $conf = $media::type($short_type);                              
                $conf['name'] = $short_type;                                    
                if ($media::match($this, $conf)) {                              
                    $_type = $short_type;                                       
                    break;                                                      
                }                                                               
            }                                                                   
        }                                                                       
    }                                                                           
    return ($this->_type = $_type);                                             
}

This will correctly determine a single short name to use for the type of the request data, and it can now correctly decode the request body.

gscherer added 4 commits July 19, 2018 13:21
…Media` with

the same content types, even when distinguished by `conditions`, an instance of
`lithium\net\http\Request` will fail to determine the correct type to use to
decode the request body. For example, if we register another type which use
'application/json' as a content type like this:

```php
use lithium\net\http\Media;

Media::type('json_base64', ['application/json'], [
    'encode' => function ($data) {
        return base64_encode(json_encode($data));
    },
    'decode' => function ($data) {
        return json_decode(base64_decode($data), true);
    },
    'cast' => true,
    'conditions' => [
        'http:content_transfer_encoding' => 'base64'
    ]
]);
```

In this case, a call to `Media::type('application/json')` will return an array
like:

```php
[
	'json_base64',
	'json'
]
```
This case is not handled by `lithium\net\http\Message::type`, which causes
`lithium\net\http\Request::$_type to be assigned as the content type of
'application/json'.

Futhermore, when `Request::body` is called to decode the request body, the
Media class fails to find a handler for 'application/json' (it expects a short
name like 'json').

To fix this, `lithium\net\http\Message::type` must be extended by
`lithium\net\http\Request`. In the case that the type is still a full content
type like 'application/json', we can loop over each short name returned by
`Media::type` and attempt to match the request like this:

```php
public function type($type = null) {
	$type = parent::type($type);
	$media = $this->_classes['media'];
	if (strpos($type, '/') !== false) {
		$data = $media::type($type);
		if (is_array($data) && !isset($data['content'])) {
			foreach ($data as $short_type) {
				$conf = $media::type($short_type);
				if ($media::match($this, $conf)) {
					$type = $short_type;
					break;
				}
			}
		}
	}
	return $this->_type = $type;
}
```

This will correctly determine a single short name to use for the type of the
request data, and it can now correctly decode the request body.
@gscherer gscherer changed the title In the case where multiple types are registered to `lithium\net\http\… Detect correct type on request, when there a multiple types mapped to the same content type Jul 19, 2018
@gscherer gscherer changed the title Detect correct type on request, when there a multiple types mapped to the same content type Detect correct type on request when there a multiple media types mapped to the same content type Jul 20, 2018
`lithium\net\http\Media::match`, which expects the Request::get() method to
be available.
@gscherer gscherer changed the title Detect correct type on request when there a multiple media types mapped to the same content type Detect correct type on request when there are multiple media types mapped to the same content type Jul 20, 2018
@nateabele
Copy link
Member

@mariuswilms I don't recall, is this on the critical path for the request/response loop? If so, it should probably be benchmarked for performance impact.

@gscherer
Copy link
Contributor Author

gscherer commented Jul 25, 2018

This issue also exists in this situation when attempting to find the correct type on \lithium\net\http\Response given only the Content-Type header. I only implemented this fix on the Request however, since it depended on Media::match, which in turn expects an instance of \lithium\action\Request as an argument.

Ideally, this would be a part of \lithium\net\http\Message::type(), but it would require that we have the functionality of \lithium\action\Request on the Message class (e.g. is() and get()).

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.

2 participants