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

Improved linked items, nested documents #10

Closed
wants to merge 9 commits into from

Conversation

lphuberdeau
Copy link

Playing around with the library, I ran into issues working with nested documents. Seemed like given all the meta-data available, having to collect the linked documents was un-necessary.

  • I added a collect argument to automatically gather the linked block while replacing them with identifiers.
  • I added recursion into the other encountered resolvers so that they get the same collection behavior.

At this time, there is a missing entry in links that should get autopopulated. All available links are searched for in nested documents. It should likely be the same behavior for top level.

Are you open to such changes?

@kalasjocke
Copy link
Owner

Interesting idea and thanks for your contribution, I'm up for changes!

I've always wanted to keep the data fetching and the serialisation logic apart. To me it's not obvious that you always have a model layer with populated relations upon serialisation. That's why I kept them separate. But then on the other hand I kind of like the fact that we can use meta data to traverse the graph when you do we have a populated model layer.

How do you solve the problem of having 1 + N database queries when using this? Do you run some function to populate all relations up front before serialising?

@lphuberdeau
Copy link
Author

How we get to the input data structure is an entirely different subject. In my case, the second nesting can be obtained as part of the same query. It's just a matter of packing the data structure in a meaningful way.

I'll get to some finishing touches later this week.

@kalasjocke
Copy link
Owner

I guess it's a different subject yes, I was just curious on how you solved it in your case. 😄

Looking forward to your finishing touches and to merge this pull request.

@lphuberdeau
Copy link
Author

This should be about complete.

  • I separated the collection from the links argument, it will now attempt to collect everything.
  • The links block now gets generated based on which of the links are being collected.
  • Linked items get sorted, duplicates are ignored.
  • Re-factored the code a little bit to avoid having two pieces of code for formatting results.
  • Added a bit of documentation.

@@ -80,6 +80,22 @@ json = PostResponder.respond(post, linked={'comments': post['comments']})

```

Alternatively, the linked items can be automatically collected. The resulting structure will be the same.
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe you could mention how they'll be collected.

@kalasjocke
Copy link
Owner

Great stuff @lphuberdeau, I like the general idea, I just had some comments on naming. Perhaps we can invest some more time on make the code a bit more readable?

@lphuberdeau
Copy link
Author

Turns out most of this is useless now as the JSONAPI spec changed entirely overnight.

@kalasjocke
Copy link
Owner

@lphuberdeau Yeah, bah. 😢

@kalasjocke kalasjocke closed this May 31, 2015
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.

None yet

2 participants