You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is kind of a wild edge case, but when a nested, valid include is defined, the QueryParser throws an error when that relationship is being included because the atom representation of the nested relationship has not yet been loaded in BEAM.
When calling to the controller with the include=songs.artists query param, a 500 error will be returned because of runtime loading of modules/atoms. Specifically this line in handle_nested_include/3:
While I understand the reasoning for the current implementation (we don't want to cast external params to atoms and allow for DOS memory attacks), I think we can approach the problem differently.
Instead of converting the external params to match the internal setting. Could we convert the internal setting to match the incoming params. So instead of using a word list to define valid includes, could we use atoms to begin with and then cast them to strings and compare those strings with the ones from the internet? That way the atoms present in BEAM as soon as the module is loaded.
So instead of using a word list to define valid includes, could we use atoms to begin with and then cast them to strings and compare those strings with the ones from the internet?
Something along those lines is probably preferable to the existing implementation in more than one way. We need to decide if it is good or bad (or unimportant) that the existing implementation takes nested include options literally (songs.artists means "including artists on songs is allowed" not "including songs is allowed and including artists on songs is allowed"). The current behavior was a side effect of the quickest implementation possible, rather than a carefully considered decision.
I'd like to start planning for a v2 release of the library so we can introduce some breaking changes and this could be one of them.
This issue has been automatically marked as "stale:discard". We are sorry that we haven't been able to prioritize it yet.
If this issue still relevant, please leave any comment if you have any new additional information that helps to solve this issue. We encourage you to create a pull request, if you can. We are happy to help you with that.
This is kind of a wild edge case, but when a nested, valid include is defined, the
QueryParser
throws an error when that relationship is being included because the atom representation of the nested relationship has not yet been loaded in BEAM.Example
When calling to the controller with the
include=songs.artists
query param, a 500 error will be returned because of runtime loading of modules/atoms. Specifically this line inhandle_nested_include/3
:jsonapi/lib/jsonapi/plugs/query_parser.ex
Line 258 in 00d01cf
Proposed Solution
While I understand the reasoning for the current implementation (we don't want to cast external params to atoms and allow for DOS memory attacks), I think we can approach the problem differently.
Instead of converting the external params to match the internal setting. Could we convert the internal setting to match the incoming params. So instead of using a word list to define valid includes, could we use atoms to begin with and then cast them to strings and compare those strings with the ones from the internet? That way the atoms present in BEAM as soon as the module is loaded.
This may look something like this:
The text was updated successfully, but these errors were encountered: