Closed matthewjumpsoffbuildings closed 3 years ago
Can you share the filter used for to replicate the issue?
Not really, its a complex setup with a lot of filters.
The resource I am viewing has two other resources attached to its detail page via a HasMany and a HasManyThrough
One of the attached resources has the following filters
return [
'verified' => new VerifiedTrackFilter,
'official' => new OfficialTrackFilter(true),
'favourite' => new FavouriteFilter,
'user' => new UserFilter,
'gender' => new GenderFilter,
'state' => new StateFilter,
'age' => new AgeRangeFilter,
'date' => new DateFilter('Vote Date Range Filter', 'votes'),
'active' => new ActiveFilter('votes'),
'advanced' => new AdvancedVoteFieldSearch
];
And the other has
return [
new UserFilter,
new StateFilter,
new GenderFilter,
new AgeRangeFilter,
new DateFilter('User Date Range', 'users'),
new AdvancedUserFieldSearch
];
Without sharing the entire project, theres not much more I can say. Did you look at the gist I shared of the filter query params? Here is a gist of the unencoded version of the filter query params
https://gist.github.com/matthewjumpsoffbuildings/2ab9e3b50494995c27d594c4473428a4
filter URL can't just be copy between different app, it require both to have the same filters. Even if I have the filter I need to be able to recreate the issue locally before I can submit a fix. Right now I'm not sure whether it's due to length of the filter string causing an issue (don't seem so since you can decode it) or the filters itself causing 502.
Of course I understand that you cant use my filter URL. I was hoping that you might be able to see an issue in the gist of the URL or the gist of the decoded URL
its the filter length. I did a test
/admin/resources/ips/18?test=W3siY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXFVzZXJGaWx0ZXIiLCJ2YWx1ZSI6eyJzdWJzY3JpYmVkIjpmYWxzZSwiY29tcGV0aXRpb24iOmZhbHNlLCJjYWxsIjpmYWxzZSwic2luZ2xlcyI6ZmFsc2UsIm5vbi12b3RlZCI6ZmFsc2V9fSx7ImNsYXNzIjoiQXBwXFxOb3ZhXFxGaWx0ZXJzXFxTdGF0ZUZpbHRlciIsInZhbHVlIjoiIn0seyJjbGFzcyI6IkFwcFxcTm92YVxcRmlsdGVyc1xcR2VuZGVyRmlsdGVyIiwidmFsdWUiOiIifSx7ImNsYXNzIjoiQXBwXFxOb3ZhXFxGaWx0ZXJzXFxBZ2VSYW5nZUZpbHRlciIsInZhbHVlIjpbMCwxMjBdfSx7ImNsYXNzIjoiQXBwXFxOb3ZhXFxGaWx0ZXJzXFxEYXRlRmlsdGVyIiwidmFsdWUiOiIifSx7ImNsYXNzIjoiQXBwXFxOb3ZhXFxGaWx0ZXJzXFxBZHZhbmNlZFVzZXJGaWVsZFNlYXJjaCIsInZhbHVlIjoiIn1dW3siY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXFZlcmlmaWVkVHJhY2tGaWx0ZXIiLCJ2YWx1ZSI6IiJ9LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXE9mZmljaWFsVHJhY2tGaWx0ZXIiLCJ2YWx1ZSI6IiJ9LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXEZhdm91cml0ZUZpbHRlciIsInZhbHVlIjoiIn0seyJjbGFzcyI6IkFwcFxcTm92YVxcRmlsdGVyc1xcVXNlckZpbHRlciIsInZhbHVlIjp7InN1YnNjcmliZWQiOmZhbHNlLCJjb21wZXRpdGlvbiI6ZmFsc2UsImNhbGwiOmZhbHNlLCJzaW5nbGVzIjpmYWxzZSwibm9uLXZvdGVkIjpmYWxzZX19LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXEdlbmRlckZpbHRlciIsInZhbHVlIjoiIn0seyJjbGFzcyI6IkFwcFxcTm92YVxcRmlsdGVyc1xcU3RhdGVGaWx0ZXIiLCJ2YWx1ZSI6IiJ9LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXEFnZVJhbmdlRmlsdGVyIiwidmFsdWUiOlswLDEyMF19LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXERhdGVGaWx0ZXIiLCJ2YWx1ZSI6IiJ9LHsiY2xhc3MiOiJBcHBcXE5vdmFcXEZpbHRlcnNcXEFjdGl2ZUZpbHRlciIsInZhbHVlIjoxfSx7ImNsYXNzIjoiQXBwXFxOb3ZhXFxGaWx0ZXJzXFxBZHZhbmNlZFZvdGVGaWVsZFNlYXJjaCIsInZhbHVlIjoiIn1d
And I get the 502. Obviously its an issue with nginx not allowing query strings of that length
And I get the 502. Obviously its an issue with nginx not allowing query strings of that length
502 doesn't always mean query string length.
it could also happen when compiling a huge query and server can't manage it (yes happened to me in previous projects). So within additional information it hard to move forward with this.
I literally just passed ?test=SOME_LONG_GIBBERISH
I just checked my nginx logs
020/12/03 01:20:41 [error] 244#0: *11721 upstream sent too big header while reading response header from upstream
Its filled with these errors. So, the filter query string breaks nginx if it gets too long. You have all the info you need
Also this is a 502 bad gateway nginx error. Its not even hitting php/laravel, i see no logs in my laravel logs, its breaking at the nginx level. The issue is 100% the length of the query parameter.
Im guessing its not just the query params, nginx likely has a limit on the combined header size, including cookies, session data and query params
Tested with the latest query string and indeed manage to cause the error, but not the first one.
However, it's in theory hard to change filters structure without introducing a major breaking change at the moment. Filter contains the actual class name as reference and it is done to allow filter to be used from App
or any packages, so it doesn't have any uriKey()
unliked resources, lenses or action.
We're open to any suggestion on how to handle this.
In the meantime, I tested some configuration provided based described in https://github.com/laravel/valet/issues/290#issuecomment-582826190 and it seem to work.
fastcgi_buffer_size 4096k;
fastcgi_buffers 128 4096k;
fastcgi_busy_buffers_size 4096k;
Yea I noticed that the param was encoding the entire qualified classnames in there. I guess one option would be if the class is in the default \\App\\Nova\\Filters\\
namespace, you could just use the class name? Eg if its \\App\\Nova\\Filters\\UserFilter
it would just be UserFilter
, which you would detect doesnt begin with \\
so you can assume its the default namespace. Then only include the fully qualified classname if its not located in that namespace? Surely 80% of the time thats where the filters will be right?
Changing the config on my local valet nginx is all well and good, but Im using Google App Engine Standard environment in production, and apparently there is no where to change these nginx settings I am aware of.
There must be another way to encode the filters that isnt so verbose and runs this risk...
I guess one option would be if the class is in the default
\\App\\Nova\\Filters\\
namespace, you could just use the class name?
If we do this, if someone bookmark URL with filters, the URL will be broken (possible JavaScript error and empty nova dashboard screen) but the benefits of this will reduce a lot of characters.
I considered this:
abstract class Filter
{
public function uriKey()
{
return self::class;
}
}
Where you can override the class method if you need it (avoid breaking change but require additional work each time). But this all just theory and I need to test it out.
@crynobone thats not true - im suggesting that if a filter string starts with \\
nova will parse it as a full class name. So even if they have \\App\\Nova\\Filters\\SomeFilter
in their bookmarks, it will continue to work fine
However if the string does not start with \\
- eg SomeFilter
- I am suggesting that nova automatically append \\App\\Nova\\Filters\\
to it
This seems like a backwards compatible approach?
And I am also suggesting that going forwards, novas vue components take advantage of this underlying php parsing, and when generating the query string on the vue side, use the unqualified class name if the filter does indeed live in the default namespace.
In fact this approach would even work if the filter is nested inside \\App\\Nova\\Filters\\SomeNestedNameSpace\\SomeFilter
All that would have to happen is that the nova vuejs side use SomeNestedNameSpace\\SomeFilter
in the query string, and the nova php side would convert it to the full \\App\\Nova\\Filters\\SomeNestedNameSpace\\SomeFilter
since the string didnt start with \\
thats not true - im suggesting that if a filter string starts with \ nova will parse it as a full class name. So even if they have \App\Nova\Filters\SomeFilter in their bookmarks, it will continue to work fine
If we look at your example above, the filter doesn't prefix with \
for all classes. So if we do based on your suggestion it will cause breaking change.
users_filter: [
{
"class": "App\\Nova\\Filters\\UserFilter",
"value": {
"subscribed": false,
"competition": false,
"call": false,
"singles": false,
"non-voted": false
}
},
{
"class": "App\\Nova\\Filters\\StateFilter",
"value": ""
},
{
"class": "App\\Nova\\Filters\\GenderFilter",
"value": ""
},
{
"class": "App\\Nova\\Filters\\AgeRangeFilter",
"value": [
0,
120
]
},
{
"class": "App\\Nova\\Filters\\DateFilter",
"value": ""
},
{
"class": "App\\Nova\\Filters\\AdvancedUserFieldSearch",
"value": ""
}
]
e.g: App\Nova\Filters\UserFilter
will become App\Nova\Filters\App\Nova\Filters\UserFilter
The vue component is generating that? Why cant it prefix all fully qualified class names with \\
going forwards?
In which case it would be: (note \\Some\\Other\\Namespace\\SomeFilter
is the only fully qualified one)
users_filter: [
{
"class": "UserFilter",
"value": {
"subscribed": false,
"competition": false,
"call": false,
"singles": false,
"non-voted": false
}
},
{
"class": "\\Some\\Other\\Namespace\\SomeFilter",
"value": ""
},
{
"class": "GenderFilter",
"value": ""
},
{
"class": "AgeRangeFilter",
"value": [
0,
120
]
},
{
"class": "DateFilter",
"value": ""
},
{
"class": "AdvancedUserFieldSearch",
"value": ""
}
]
``
@matthewjumpsoffbuildings I trying to solve this without making a breaking change.
Eg preserving old bookmarks?
Unfortunately I dont think theres a way to sort this without making old bookmarks stale.
I was also going to suggest changing the common "class"
and "value"
keys. They are redundant. Something like this:
[
"AgeRangeFilter",
[
0,
120
]
],
would be way more efficient given that nginx has this kind of limitation. Note each filter is simply an array with 2 indexes, [0]
is the class name, [1]
is the value
If possible, otherwise this no longer a small feature/bugfixes and I need to make a proposal to the team and get the approval.
Fair enough. I was wondering if I could make a pull request? Is it possible I would be able to have access to the repo, or is that not an option?
Is it possible I would be able to have access to the repo, or is that not an option?
The only way to get access to the repository is through https://nova.laravel.com/docs/3.0/contributions.html#contributing
Oh right, I didnt even see that option, thanks.
Any chance you could point me in the direction of the files on the Vue side that handle generating the query string, and the files on the PHP side that handle parsing/using it? I could probably suss it myself but if you can point me in the right direction would be appreciated
I see the Vue side occurs here https://github.com/matthewjumpsoffbuildings/nova/blob/3.0/resources/js/store/resources.js, just looking for the PHP side
@crynobone I made a pull request https://github.com/laravel/nova/pull/1117 that does what Im talking about, its not passing the style CI checks but I cant get style CI working for some reason.
Here's my alternative solution as suggested above, which should work.
Wont you also need to update the vuex store behaviour so it encodes the filter params using the shorter names?
Wont you also need to update the vuex store behaviour so it encodes the filter params using the shorter names?
Filter will set class
from Filter::key()
, so there no reason to check the frontend behaviour.
Cool, tested that and it seems to work great for shortening the class name.
Do you think its feasible to consider also removing the redundant class
and value
from the json?
[{"class":"UserFilter","value":"test"},{"class":"DateFilter","value":"1"}]
vs
[["UserFilter","test"],["DateFilter","1"]]
Considering the default nginx config has quite a strict limit on the query string length, the more it can be shortened the better. My pull request also contained an attempt to make that change
Do you think its feasible to consider also removing the redundant class and value from the json?
Breaking changes
Yea I get it, but I think considering the query string limitation nginx has, and that things like Google app engine are running it under the hood without the option to change the limit, at some point it would be smart to make the switch. Having the words class
and value
in there serves no purpose, and almost doubles the length of the query param string
@crynobone just saw your pull request, looks great!
The PR has been reverted so this issue will still persist in upcoming version.
I've just been stung with this all morning.
For anyone coming and just wants to get on with fixing this and move on here's what I did. I'm using laravel Valet, but this is a nginx issue at heart.
Looking at my nginx.conf file in /usr/local/etc/nginx/nginx.conf
it shows that the it reads the following directories for config files:
include "/Users/jonathan/.config/valet/Nginx/*";
include servers/*;
include valet/valet.conf;
I created a new file in /usr/local/etc/nginx/servers
called buffers.conf
In this file i put this exactly:
fastcgi_buffers 16 32k;
fastcgi_buffer_size 64k;
fastcgi_busy_buffers_size 64k;
proxy_buffer_size 128k;
proxy_buffers 4 256k;
proxy_busy_buffers_size 256k;
Saved the file and RESTART nginx. That should be it.
This WILL apply to all your servers defined in nginx.
Hope this gets fixed for next release.
Description:
I am finding that when the URL has a really long query string parameter on a detail page with several HasMany/Through relationships, since each filter for each of the related resources is appended to the query, when I reload the page I get a 502 Bad Gateway from nginx. Both on my local dev machine, as well as in Google App Engine.
Heres a gist of the kind of query string that is triggering the 502 - https://gist.github.com/matthewjumpsoffbuildings/e76181eb9d311f048d5692b269fe4b5d
I find if I simply delete the query string then it works again. Is there a way to turn off this query string behaviour? Or restrict its length?