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

Implement sparse fieldset #273

Closed
wants to merge 2 commits into from
Closed

Conversation

ghunti
Copy link
Contributor

@ghunti ghunti commented Feb 7, 2016

Hi.

This is a first proposal to implement the sparse fieldset feature. The idea is to discuss the current implementation to improve on what was done :-)

This are the "major features" in order to respect JSON API spec:

  • If no field param is specified current behavior is kept
  • If one or more fieldsets are provided then, only the fields on those fieldsets are returned (only exception to this is the id field, that will NEVER be removed)
  • If one or more fieldsets are provided, relationship names MUST also be provided, otherwise the relationship object will not hold those relations

To mimic the behavior of includes, we need to call a method parseFieldsets() in order filter the response fields.

The fieldset filter is respecting the JSON API, but it's also affecting the other serializers (just like the included relations work right now)

Don't accept the commit, because it's still missing unit test

@willishq
Copy link
Member

willishq commented Mar 9, 2016

Hey, I'll merge this in if you write some tests for it! Thanks!

@dennisoderwald
Copy link

+1

@ghunti
Copy link
Contributor Author

ghunti commented Mar 28, 2016

@willishq @dennisoderwald Working on them, but been very busy lately.

@ghunti
Copy link
Contributor Author

ghunti commented Apr 2, 2016

Unit tests pushed.

@mdwheele
Copy link

mdwheele commented Jul 6, 2016

@willishq Any updates on the merge-ability of this feature?

@marcaube
Copy link

@greydnls it seems like @willishq is missing in action 😛 What's the status of this PR ?

@LavaToaster
Copy link

@greydnls @willishq @philsturgeon Is there anything blocking this? (Not sure who to ping, sorry!)

@Indemnity83
Copy link

Would really love to use this in a project under development; its been months with no update though 😞

@greydnls
Copy link
Contributor

I'm sorry about the delay on this, life has been hectic. This PR looks good and the tests are passing. I need to resolve conflicts and I'll merge it.

@rollbackpt
Copy link

+1
I was just looking for this feature on the documentation. Any predictions on this merge?

@greydnls greydnls closed this Feb 22, 2017
@greydnls
Copy link
Contributor

Thank you @ghunti 👏 👏 This was merged in manually, not sure why GH didn't mark the PR as closed. Changes are on master. I'll cut a new release shortly.

@ghunti
Copy link
Contributor Author

ghunti commented Feb 22, 2017

Thanks for everything @greydnls !!!

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

9 participants