Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Can't return a collection object with embedded entities when content negotiation is set to Json #31

Merged
merged 3 commits into from
Jul 16, 2014

Conversation

weierophinney
Copy link
Member

Possible resolution as mentioned below:

okay, it looks like we don't allow you to replace the collection at any point currently! I think we may need to change that.

Transcript from IRC:

<djule5> weierophinney: on a resource's fetchAll with Json content negotiation, how would you return a collection of entities with extra attributes (e.g. total_items, as with HalJson)?
<weierophinney> djule5: wrap them as an array embedded in another object, and have those properties either as public, or make that object JsonSerializable.
<djule5> weierophinney: that's exactly what I'm doing, but the api call returns me an empty Json array... I was  confused because I knew I was returning the data correctly at the end of the fetchAll call... but I just found out what's happening, and it's in ZF\Rest\Resource::fetchAll()... basically if the returned value from the fetchAll call is not an array, HalCollection or Traversable, an empty array is returned. Is that the correct behavior?
<weierophinney> djule5: yes -- because RestController will then cast the array to a HalCollection.
<djule5> weierophinney: so how is your suggestion supposed to work in this case? If I return an object, an empty array is being returned (what I'm getting right now)
<weierophinney> djule5: hmmmm... trying to figure out a way to manipulate it. I thought you'd be able to via the Resource's events or the RestController's events, but I'm not seeing a way. Going to look in zf-hal in a momemnt.
<weierophinney> djule5: okay, it looks like we don't allow you to replace the collection at any point currently! I think we may need to change that. Can you raise an issue on each of zf-rest and zf-hal about that for me?
<djule5> weierophinney: that's what I thought too! ;) and I'd really like to be able to use collection objects to accomplish this. So yes I will raise the issue on both repos :) thanks!

@weierophinney
Copy link
Member

I've been looking at the use case, as well as the code changes necessary, and I'm hitting some stumbling blocks.

Essentially, my original idea was that you could return an object that composes a collection as a property, and zf-hal would serialize that. However, there are a few problems here:

  • First, and most importantly, right now, ZF\Rest\Resource returns an empty array if the object returned by the listener is not a Hal Collection, ApiProblem, ApiProblemResponse, Traversable, or array. This automatically invalidates my original idea.
  • Second, ZF\Rest\RestController examines the returned argument; for ApiProblem or ApiProblemResponse, it immediately returns it. For a Hal Collection, it will inject the self link (this is done as part of the createCollection() call. It then assumes that Resource returned an array or Traversable at this point, and passes it to the plugin's createCollection method -- and in each case, the object will be passed to the Collection constructor... which expects an array or Traversa ble. So, this also invalidates the idea.
  • Third, the Hal plugin's renderCollection() method in turn invokes extractCollection()... which iterates over the original object... which means it must be an array or Traversable if that is not to throw an error.

My thought is this: allow Resource::fetchAll() to return any object, and RestController::getList() to cast non-array/non-Traversable objects to a ZF\Hal\Entity (i.e. createEntity() instead of createCollection()). This would allow you to do as I suggested -- you could compose another object that is listed in the metadata map as a collection as a property, which would allow serialization as an embedded collection -- but also allow serialization to JSON.

I'm going to take this route now; let me know if you do not think this will work.

- Modified `Resource::fetchAll()` to allow returning arbitrary objects
- Modified `invalidCollection()` data provider to remove case using
  `stdClass`, as that case is invalidated.
- Renamed `testFetchAllReturnsEmptyArrayIfLastListenerDoesNotReturnArrayOrTraversable`
  to `testFetchAllReturnsEmptyArrayIfLastListenerReturnsScalar` to
  clarify new intent.
weierophinney added a commit to weierophinney/zf-hal that referenced this pull request Jul 16, 2014
- Actually, `ZF\Hal\Entity` allowed it previously; the new test passed
  even without changes. Simply modified the code to explicitly make the
  `$id` an optional argument.
weierophinney added a commit to weierophinney/zf-hal that referenced this pull request Jul 16, 2014
- Do not return an ApiProblem in the case that an entity's identifier
  was not discovered.
- Updated `injectSelfLink()` to not include null identifiers for
  entities.
- Modified `getList()` to look for non-array, non-`Traversable`,
  non-`ZF\Hal\Collection` object returns from `Resource::fetchAll()`,
  and to pass those to the `createEntity()` of the Hal plugin instead of
  `createCollection()`.
- Updated `create()` to check for a null identifier on the returned
  entity and explicitly return an ApiProblem in that situation (the
  previous behavior).
@jdelisle
Copy link
Author

Matthew, yes it sounds like it would work. I'll try it out in my implementation once it's ready, thanks!

- Will change this to `>=1.0.2,<2.0` when zf-hal is tagged at 1.0.2.
@weierophinney
Copy link
Member

@jdelisle Okay, changes are in both zf-hal and zf-rest. To test, modify your application's composer.json:

  • Add a repository key, and tell it to use mine for zf-rest:

    "repositories": [
        {
           "type": "vcs",
           "url": "https://github.com/weierophinney/zf-rest.git"
        }
    ]
  • Under require, add the following:

    • "zf-rest": "dev-hotfix/31 as 1.0.3"
    • "zf-hal": "dev-master as 1.0.2"

@jdelisle
Copy link
Author

@weierophinney I've tried exactly what you said, but for some reason composer returns me this:

composer update --dry-run
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - The requested package zf-rest could not be found in any version, there may be a typo in the package name.
  Problem 2
    - The requested package zf-hal could not be found in any version, there may be a typo in the package name.
  Problem 3
    - Installation request for zfcampus/zf-apigility-skeleton 1.0.x-dev -> satisfiable by zfcampus/zf-apigility-skeleton[1.0.x-dev].
    - zfcampus/zf-apigility-skeleton 1.0.x-dev requires zf-rest dev-hotfix/31 as 1.0.3 -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://groups.google.com/d/topic/composer-dev/_g3ASeIFlrc/discussion> for more details.

Read <http://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

@jdelisle
Copy link
Author

@weierophinney Nevermind, I figured that one out:

"zfcampus/zf-rest": "dev-hotfix/31 as 1.0.3",
"zfcampus/zf-hal": "dev-master as 1.0.2"

@weierophinney
Copy link
Member

@jdelisle cool -- let me know if this works for you!

@jdelisle
Copy link
Author

@weierophinney Ok, so I've been testing this a bit and here's what I get.

See my gist here: https://gist.github.com/jdelisle/0fda5fee208ea35e9117

In module.config.php, I'm pretty much using the default config apigility is providing me with. If I use that, it fails and I get the stack trace shown in stack-trace.json. It ends up going back in the \ZF\Hal\Collection constructor.

If however I take out the entry for the collection class in the metadata_map, I get the payload in payload.json and it seems to be working fine. But when I go back to the admin and save the resource, it creates the collection class entry back in the metadata_map.

Am I not using it the way you intended? If so, could you provide me with a working example?

@weierophinney weierophinney merged commit 7417e07 into zfcampus:master Jul 16, 2014
weierophinney added a commit that referenced this pull request Jul 16, 2014
- Updated zf-hal to 1.0.2 (to pick up required changes in the hal plugin)
weierophinney added a commit that referenced this pull request Jul 16, 2014
weierophinney added a commit that referenced this pull request Jul 16, 2014
@weierophinney weierophinney deleted the hotfix/31 branch July 16, 2014 21:19
@weierophinney weierophinney added this to the 1.0.4 milestone Jul 16, 2014
@jdelisle
Copy link
Author

Funny... just read zfcampus/zf-hal#38 (comment) and it seems related to what I've just described in my previous comment...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants