Include filter does not work
Closed this issue · 16 comments
It is not possible to include a remote model's relation. For example, consider models called Trip and Country, where:
TripbelongsToCountry- These models exist in both a local app and a remote LB app.
- In the local app, these models are backed by a datasource configured w/ the remote connector.
When a query is performed with an include filter (Trip.findOne({include: 'country' }), a remote call is made: GET /api/Trips/findOne?filter[include]=country. The remote service dutifully returns the Trip instance with an additional country property. Despite this, the resulting instance on the local app does not contain the relational data.
The reason for this is straight forward - loopback-connector-remote coerces result data into the target model by simply instantiating it (new Model(payload)), which for strict models will strip any unrecognized properties. This line: https://github.com/strongloop/loopback-connector-remote/blob/master/lib/remote-connector.js#L83
The solution should be to expand the type handler to prime the relation cache (__cachedRelations), allowing the resulting model to track the relational data.
Guys - I actually have put together a patch that fixes this in the way described above, but I've reached some problems that run up against the premise of how this connector is implemented and want to pause for advice.
As stated above, I'm simply improving the type handler to inject relational data into the relations cache. I follow the same model that the include filter uses in the juggler:
instance.__cachedRelations[relationName] = relatedValue;
instance.__data[relationName] = relatedValue;
instance.setStrict(false);
This is all fine and good, and it generally works for JSON serialization. However, I came to realize, these model instances from the remote connector actually don't access __cachedRelations - because the connector is bypassing so much of the DAO, there's no caching on relation methods (tripInstance.country(...)), among whatever else may be lost.
Is this by design? Is it not possible to rely on the juggler to handle the relations?
Hey @doublemarked, thanks for the contribution. I'll throw a bug label on here and I started the test build on your PR.
I would suggest that you refer to your second comment here in the PR itself as the reviewer should be a little more familiar with the original design justifications (I wasn't working on the framework at the time).
Cheers
Hey @richardpringle - ok, thanks!
Hi, I also bumped into this bug these last couple of days.
I looked into how the juggler works and I came across the file: include.js. Here, when you want to do an include, it does the main query (for Trip, in your example), gets the Trip IDs and afterwards, creates a subsequent query for Countries based on Trip ID.
Your approach (look into the response for the related models) works only if the API which is called is a loopback-like API which would perform the inclusion for the caller and then the caller only needs to look in the response.
**** NOT valid since the purpose of the module is to connect loopback APIs *****
If the remote API is not loopback-like, then it might not know how to interpret the "include" query.
If you think about it, your are sending the include query, internally, inside your app (which knows about include), so you shouldn't rely on the fact that the remote API knows about include as well...
**** NOT valid since the purpose of the module is to connect loopback APIs *****
In short:
- I see your solution as an optimization (a very good one)
- A generic implementation would be like the one in datasource juggler which does subsequent queries (we should find a way to rely on the the juggler as much as possible and plug in the remote call only where it's necessary, rather than overwrite the juggler (as you also stated above). This is debatable since you are only connecting loopback APIs
What's the maintainer's input on this?
Cheers!
@horiaradu I believe this connector is specifically intended for communicating with other Loopback-based APIs, and thus can make some assumptions about the availability of the filter params, etc.
Nonetheless, I've honestly become somewhat discouraged from working on the connector as the divergence from the loopback-datasource-juggler requires any improvements (like this one) to reimplement pieces from the juggler. I have a semi-functional patch that takes into account the feedback from the main devs (you can see the discussion in the PR), but as I tried to bring that patch from "semi-functional" to "fully functional" I kept running into additional broken use cases.
In our project we were using this connector to handle backend services from a public API gateway. We have since stopped using it, replacing it with a more direct mapping of incoming routes to backend service routes.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.
This is important and it should not be set to closed by the stale-bot... If you use the remote connector to ask for a complex query from a remote loopback service, the service successfully sends the data you requested, but the connector strips of result data, is wrong by definition
@mitsos1os I haven't worked on that project which used loopback in a long while, but last I checked, that was the case.
The thing is that Loopback team successfully started promoting inter-services communication with examples such as the Facade patter, but do not have proper and complete support for connectors that will make this easy. The proposed swagger connector, is quite diverging for a loopback project in my opinion, due to method rename (underscore)... The remote connector that let's you share model definition and function calls with proper args (and not with their remote swagger form) is more appropriate... So more work should be done on the connector, given the rise of microservices architecture. I believe this issue should be reopened. I will submit a pull-request when I find some time
I am reopening the issue based on @mitsos1os patch submitted to strong-remoting.
Hopefully close to this we're also currently experiencing a similar issue. Imagine you have a large inter connected network of database tables. To create a microservice CRUD structure over it you create several CRUD Loopback services which each bundle e.g. 10 tables. Now we want a CRUD service that contains tableA and tableB with a relation between them only that A and B are in two different CRUD services.
What we currently try to do in such a case is create a another CRUD Loopback service which contains the models from tableA and -B and also provides them publically. These models also contain the added relation and are connected to the original CRUD using the remote connector. This generally works with the only problem being that trying to use e.g. the relation from tableA to B (using include or /tableA/{:id}/tableB ) will just forward the request to the tableA-CRUD even though this CRUD does not know of the relation.
When we tried using the REST connector instead this use case worked but instead the filters broke.
Now basically my questions are:
Will the attached PR change something about this behavior (as it doesn't look like it will)?
If not should we consider manipulating this behavior in our projects using e.g. mixins or similar techniques or is our attempt flawed to begin with?
I hope I didn't miss the scope here and am looking forward to any input.
@ExTheSea You are right. The referenced PR what will do, is that the returned result, will actually return the included data from the filter. This will have the related tableB data inside the __data structure of the returned tableA model... However trying to call the tableA.modelBRelation() will not return the cached data from the __data property and will try to call the remote resource (as defined in the model instead).. I will take a look if I have some time to also include the expected behavior in this PR
Actually I just did!
Now we want a CRUD service that contains tableA and tableB with a relation between them only that A and B are in two different CRUD services.
If your two tables are exposed by two different CRUD services, and each service can access only a single table (the other related table is not configured with a model), then I am afraid I don't see any way how to fetch data from both tables at the server (CRUD service) side - you will always have to do two queries to both services.
Now the question is whether LoopBack can make those multiple calls to you? I think it may be possible, if you attach your tableA model to datasourceA (pointing to your CRUD service exposing tableA) and attach your tableB model to datasourceB (pointing to your CRUD services exposing tableB). Have you tried that, @ExTheSea?
Having said that, let's move this discussion to a new issue, as it's a different one from the problem described at the top of this issue.
I am closing this one as fixed, please update your dependencies to loopback-connector-remote@3.3.0 (or newer).
@bajtos (and @mitsos1os if there is interest): I tried to elaborate on my issue in #84