Multiple filter w/ include fix + internal routing fix + more #233
Conversation
…quest object; small swagger validation fix for when using prefixalias; limit url for internal resource fetching to basepath onwards
|
There was also an issue where setting graphiql as false would still run the joiconverter process to build the definition. So now if the param is false it skips executing the process. The API I'm building has attributes that are objects which throws errors with the Joi to GraphQL converter so maybe it's something to be fixed, but in the interim I'd just like to have it off. |
|
@pmcnr-hx would you like to take a review on this one? You were the closest to the filter preprocessing a few months back. I'm fine with the idea of turning off all of GraphQL with a strict |
|
I'll be reviewing this soon, but the changes are not trivial, so I'll need to take some time. |
|
Is there anything I can do to help you along? |
|
I'll review today @st3xupery. Meanwhile can you merge or rebase on master? |
|
@pmcnr-hx By that what do you mean? Do you want me to make my pull request off of the master of my fork? I'm still using the master for legacy support of one of my projects. I can make another fork if need be. |
|
@st3xupery. What you just did. Thanks. I'm moving this to the top of my TODO list. Should be able to get round to it soon. Apologies for the delay. |
|
@pmcnr-hx Okay, great and no worries~! |
pmcnr-hx
left a comment
pmcnr-hx
left a comment
There was a problem hiding this comment.
Apologies for the delays reviewing this @st3xupery. Some initial comments. I'm going to have a more detailed look to see how easily we can use the processed filter in the internal routing logic.
| for (const key in request.params.filter) { | ||
| filterElement = request.params.filter[key] | ||
|
|
||
| if (typeof filterElement === 'string') request.params.filter[key] = filterElement = filterElement.split(FILTER_SEPERATOR) |
There was a problem hiding this comment.
This is a pragmatic solution, but I'm not too thrilled about mutating request.params.filter[key]. I'm going to have a look at the internal routing and check if we can used the processed filter there also.
| schema: jsonApiGraphQL.generate(jsonApi._resources), | ||
| graphiql: !!config.graphiql | ||
| })) | ||
| if (config.graphiql !== false) { |
There was a problem hiding this comment.
Not sure what you mean, is !== false not a strict equivalency?
There was a problem hiding this comment.
What we were suggesting was a new configuration property named strict, but we can get round to doing that in a later PR.
| let basePath, host, protocol | ||
| if (jsonApi._apiConfig.urlPrefixAlias) { | ||
| const urlObj = url.parse(jsonApi._apiConfig.urlPrefixAlias) | ||
| basePath = urlObj.pathname.replace(/\/$/, '') |
There was a problem hiding this comment.
If we use the (?<!^)\/$ regexp (with a look behind to assert we're not at the beginning of the string) above we can avoid the if.
There was a problem hiding this comment.
Had to use something slightly different as javascript does not have a look behind in it's regex capabilities. The alternative works to the same effect: (?!^/)/$
|
Hi, this fix is kind of crucial for us. It's blocking us from upgrading to latest version, thus we stuck with old bugs. When should we anticipate for this merge to take place? |
|
It seems like there are no big objections to what's going on here. As soon as the build goes green, I'll merge and release. Looks like the last commit broke something, it looks unrelated to the rest of this pr, so if the build is still failing tomorrow midday GMT I might merge this PR then revert the troublesome commit. |
|
Good news! Thanks 🎉 |
|
Hey! Sorry I kind of neglected this. |
|
I've made the regex replacement of the if statement. Had to use something slightly different as javascript does not have a look behind in it's regex capabilities. The alternative works to the same effect: |
There was a problem hiding this comment.
Looks good. Any minor details can be addressed later. Thank you for all the hard work @st3xupery getting these fixes in: 👍
Ready to be merged and released @theninj4: 
| schema: jsonApiGraphQL.generate(jsonApi._resources), | ||
| graphiql: !!config.graphiql | ||
| })) | ||
| if (config.graphiql !== false) { |
There was a problem hiding this comment.
What we were suggesting was a new configuration property named strict, but we can get round to doing that in a later PR.
4 participants
Applied a fix that was breaking the ability to acquire includes when using multiple filters through a comma separated list. It seems I was splitting the filter param by comma too far into a particular function that wasn't called when internally routing includes. You'll see the filter split happens immediately in the parseandValidate function. I wrote up two tests for this and added an additional photo resource which required me to modify the counts in a few test assertions.
Additionally in the include lib when building a list of resources to fetch I reduced the url to just the base + path of the url as the hostname is not required while routing internally. This also remedies a problem where the use of pathPrefix which is overwritten by the apiPrefixOverride would cause this route to be incorrect from an internal standpoint.
Finally I appended originalUrl to the request object which persists as the same value during rerouting as it allows me to compare the route object to the originalUrl to see if the request is the result of an include or whatever else may prompt a reroute. I feel like this is a quasi acceptable use of the param as defined in the express API here http://expressjs.com/en/api.html#req.originalUrl