json-schema icon indicating copy to clipboard operation
json-schema copied to clipboard

Addresses Content-Type mismatch on HTTP 301 Moved Permanently with FileGetContents

Open DannyvdSluijs opened this issue 1 year ago • 3 comments

This PR attempts to resolve issue #694, in order to do so three commits where added

  • The first adds a failing test showing the issue
  • The second refactors the usage of $http_response_header [link] with get_headers() [link]
  • The third commit reverse the headers before parsing to match on the latest Content-Type header.

DannyvdSluijs avatar Feb 05 '24 20:02 DannyvdSluijs

Based on our extensive test suite, I can confirm that this patch solves the reported issue :+1: Although it also exposes the performance bottleneck that comes with external references when they have to be fetched synchronously in a single-threaded fashion. :-1:

Without patch with 3.0.0-without-%24id.json

$ ./vendor/bin/phpunit --filter Async
PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.21

EEE..............SS..........SS                                   31 / 31 (100%)

Time: 00:01.500, Memory: 16.00 MB

With patch with 3.0.0.json.

$ ./vendor/bin/phpunit --filter Async
PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.21

.................SS..........SS                                   31 / 31 (100%)

Time: 00:48.817, Memory: 16.00 MB

So it is good that the problem gets fixed but for us, :100: but it is better if we stick with https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/3.0.0-without-%24id.json instead of https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/3.0.0.json.

mxr576 avatar Feb 07 '24 11:02 mxr576

So if I understand your comments correctly everything works but in your test suite you need to shift to another specification due to time constraints?

I've tried to understand the phpunit outputs and I can see the patch adds a 47 seconds time increase, but that can also be becuase of the different schema you're loading. WHat would be the Time of running with patch and 3.0.0-without-%24id.json and running without patch and 3.0.0.json as I expect the schema to be the cause and not the patch itself. At least I can think of a reason why the patch would introduce a 47 second delay. But I'm open to input in order to understand better.

In addition I've played around with the schema that is being fetched in the added test but there seems to be no impact on the timing of the test itself no matter which schema I pick.

DannyvdSluijs avatar Feb 08 '24 07:02 DannyvdSluijs

I also believe the different schema causes/caused the delay, precisely that external references have to be resolved in the https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/3.0.0.json spec synchrinously, in a blocking fashion. I have not ran a profiling on the code, but this is my strong feeling.

From this PR point of view, even if this is true, this works as designed because fixing this problem would require some refactoring - I assume. Also, admittedly, my test suite is stress testing this code because it parses several API specs with multiple data providers.

mxr576 avatar Feb 27 '24 12:02 mxr576

I don't really understand the performance problem being flagged in the comments, but it seems to me it is a separate issue and should not block this. I would recommend re-thinking the tests for this though.

IMHO this is enough of an edge case to not block 6.0 release.

This can also be the result of using the get_headers which was part of the original PR. After Jordi's comments I've reviewed and change the PR back to the original and only fixed the issue and no longer alter the logic to get the response headers.

DannyvdSluijs avatar May 26 '24 19:05 DannyvdSluijs