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

feat: relationships #44

Closed
wants to merge 19 commits into from
Closed

feat: relationships #44

wants to merge 19 commits into from

Conversation

mrnkr
Copy link

@mrnkr mrnkr commented Oct 24, 2021

Hello world!

I extended this library a bit to support relationships both in serialization and deserialization and wanted to share that implementation with y'all.

Deserialization

  1. I grab all the included resources and throw them in an ES6 Map that looks like { [resourceType-resourceId]: resourceAttributes }.
  2. For each entry found within the relationships object I add their attributes, which I get from the aforementioned Map, and their id to an object with the same name as the relationship and add it to the resulting value.

This applies for arrays as well.

Serialization

  1. The developer is expected (and they will see this in the documentation) to declare all relationships in a relationshipsMap which was added to the settings. For instance, were they to work with projects which define an owner relationship with the users resource type, then they should declare the relationship as follows:
{
  relationshipsMap: {
    projects: {
      owner: 'users'
    }
  }
}
  1. The serializer takes every entry of the object to serialize that is not an array or object and puts them in the attributes of the resource.
  2. The serializer takes every entry of the object to serialize that is an array or object and puts them in the relationships of the resource. To do that, the serializer maps them to objects that look like { type, id } and gets the type from the relationshipsMap. How so? Just getting relationshipsMap[resourceType][relationshipName] you're getting the correct resource type for the related object.

I did add tests for getting resources with included relationships (projects with included owner and collaborators which I took from an API I'm developing at the moment) and did the same for creating resources with relationships. I did not add tests for updates and deletions because, at least my API (which uses JsonApiDotNetCore) returns a 204 NO CONTENT when I send a PATCH or a DELETE, then again, I guess that is only because of the way I configured the framework/the way that particular framework works.

A use case I thought of but did not implement at first (since I personally do not need it ATM) is recursive relationships or just relationships within relationships. First case scenario could be Folders which contain folders which contain folders... Second case scenario could be just projects which have an owner which is part of an organization.

I really don't want to do anything I don't need without first discussing with you whether this approach could be improved or anything so, please, let me know what you think! I'd be happy to work on this a bit more to make it nice and complete so anyone could use it!

BTW, took the liberty of adding myself to the list of contributors at the end of the README 😄 I also added the @babel/eslint-parser so I could use optional chaining and the coalescing operator without adding linting errors hehe

Looking forward to your feedback!

Closes #10

@mrnkr
Copy link
Author

mrnkr commented Oct 27, 2021

Whenever you see this, there is another thing I've been working on in a different branch. That I did mostly because I didn't know whether this was the ideal way to do things but I still needed it for my own application.

Thing is I needed to be able to include related resources and I didn't see any way to do that other than using the relationshipsMap I defined to create an include query parameter. I haven't code any unit tests for that particular feature yet tho, I'm testing it in my application first because I need it now but if you think this is something the community could benefit from, maybe adding a bit more configurability, then I'm more than open to creating another PR or even adding it to this one

@jazomr
Copy link

jazomr commented Nov 17, 2022

👍 Plus one on this

@euoia
Copy link

euoia commented Dec 10, 2022

Just to confirm - without this PR, it is not possible to modify relationship fields on resouces? Is that right? It seems like it ought to be quite useful for this behaviour to be supported...

@mrnkr
Copy link
Author

mrnkr commented Dec 10, 2022

Just so everyone's aware, the branch I used for this PR is outdated, if some maintainer decided to review this I'd be happy to merge the latest changes I did in my development branch which fixed some issues I had while using this (all of them related to relationships, I didn't have any issues with the library itself).

@euoia
Copy link

euoia commented Dec 10, 2022

@mrnkr I think it would be great if this PR could updated with the later changes from your development branch. I would be happy to test the changes and provide any thoughts I have on them.

@henvo is there a possibility of getting this PR (or equivalent support) merged?

@mrnkr
Copy link
Author

mrnkr commented Dec 10, 2022

Done, PR is updated and conflicts are solved. @euoia if you have the time to try this out it would be great!

@euoia
Copy link

euoia commented Dec 11, 2022

I tested and I can see that the relationshipMap is adding include to the query, which is great.

All the tests pass.

With these changes, how do I use ReferenceField? I was previously doing the following:

      <ReferenceField source="companyId" reference="companies" />

But is this still correct? Wouldn't it make more sense to be like <ReferenceField source="company" reference="companies" /> since company is the name of the relationship? But that doesn't work for me (presumably because ReferenceField expects an ID and not an object).

If I leave ReferenceField with source="companyId" my listing works. But when it comes to editing, how should I use ReferenceInput? Again, using company as the source fails (presumably again because it expects an ID), but the alternative of using companyId results in a payload that contains companyId (which is not how you are supposed to update relationships in JSON:API).

I wonder how you are using this in your application.

One small thing, I was unable to run npm install without removing "@babel/eslint-parser": "^7.15.8" (which was added as part of this PR) - there was a dependency issue.

I wonder whether relationshipMap might be simpler just called relationships.

One last thing, unrelated to this PR, when my server returns an error I get a blank screen and a console error Polyglot.transformPhrase expects argument #1 to be string. Do you know if this Data Provider is correctly handling errors?

https://marmelab.com/react-admin/DataProviderWriting.html

The errors that I am getting back from https://laraveljsonapi.io/ look like this:

{
   "jsonapi":{
      "version":"1.0"
   },
   "errors":[
      {
         "detail":"The field companyId is not a supported attribute.",
         "source":{
            "pointer":"\/data\/attributes"
         },
         "status":"400",
         "title":"Non-Compliant JSON:API Document"
      }
   ]
}

@mrnkr
Copy link
Author

mrnkr commented Dec 15, 2022

My idea when I coded this was not to change anything as to the way the library works so if the docs say source in ReferenceField receives the "Name of the entity property to use for the input value" then that's what I aimed to support.

I don't think it would be a good idea to change what we get in one of these props that are already defined by the framework, especially given the abstraction layer we're working on right here.


I did some reading and here's what I found about what's going on under the hood:

If you look at the code for ReferenceInput you'll see it uses a hook called useReferenceInputController to handle all its logic.

Internally that calls the getList method of the adapter with the value passed in the reference prop as the resource name. At the same time it calls the useReference hook in order to download the currently selected value. That it does by passing the source prop to a hook called useWatch which returns a variable with the value associated with the id key within the form we're working on. That, in turn, gets passed to the getMany method of the provider.


It's possible I never actually tried to use ReferenceInput though, I think I used ReferenceField and then edited those related resources individually.

As for generating that edition payload, I see where the issue is: I used the name of the relationship in the map, not its id to check whether the key is a relationship. This makes the error occur. I'll work on adding a test for this particular scenario and making it work. Also, since I no longer have access to the project I did using this I might make a new proof of concept.

As for error handling maybe creating a separate issue for it would be the best course of action, that way we can focus on relationship support here.

@mrnkr
Copy link
Author

mrnkr commented Dec 15, 2022

So I went back to work and remembered an issue I'd had. The way getMany requests are handled is not compatible with JsonApiDotNetCore - I don't remember filtering syntax as being a part of the standard - so I made a few modifications for this to work. Now, it's clear not everyone will use the same framework I'm using nor will I use the same framework as everyone else - for that reason I think we should look into adding a configurable abstraction layer to allow users to define certain behaviors such as these.


Explanation:

The library generates a filter with a shape similar to ?filters[id][]=val1&filters[id][]=val2 and expects that to be translated by the server to an in query. JsonApiDotNetCore defines filters differently (see here). That same filter can be written as: &filters=any(id,'val1','val2') or, in legacy syntax &filters[id]=in:val1,val2.


In my opinion, for this PR to reach a "mergeable" state two things have to happen first. Firstly we should abstract query parsing so users can configure it. Secondly, and optionally, add typescript typings (no need to migrate the code, just add type declarations) to improve developer experience and remove warnings when using the lib from a ts project.

@henvo
Copy link
Owner

henvo commented Apr 28, 2023

@mrnkr @euoia thanks for your work and feedback on this. At the moment I don't have time to work on this project anymore. Are you interested in becoming a collaborator?

@henvo henvo closed this Aug 10, 2023
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.

Modifying relationships
4 participants