Move OpenAPI configuring logic

[doljae]

Description

• fix Refresh the browser each time, the global header is added in duplicate. #1820

Additional Information

• What I did was move the OpenAPI setting logic to only perform the first time after reverting.
• I guess the cause of the problem is that the update logic for OpenAPI is performed even after being cached.

[doljae]

Please tell the cause of the build failure, then I will fix it 🙂

[bnasslahsen]

@doljae,

You should have access to the build results here:

• http://129.159.254.115:8686/job/springdoc-openapi-PR/181/

If you click on details on github, you should see it

[doljae]

@bnasslahsen ,
It seems that I'm stuck in resolving test cases 😢
I think it will be solved by modifying the test data a little. Can you take a look?

[bnasslahsen]

@doljae,

Unfortunately, we cannot. We should keep the existing tests passing.
Otherwise, it will certainly create other regressions.

The only acceptable change is on TestApp138!

[doljae]

@doljae,

Unfortunately, we cannot. We should keep the existing tests passing. Otherwise, it will certainly create other regressions.

The only acceptable change is on TestApp138!

@bnasslahsen,
According to test result, my commits affect app162, 170, 184, 68. Is it possible to fix pass these cases with adjusting app138 only?

[doljae]

OK now I got the point. I found out why some tests fail.

The method I modified is AbstractOpenApiResource.getOpenApi(Locale local).
In the past, it was confirmed that the reason the duplicate header value was generated every time the browser was refreshed was that the openApiCustomisers.ifPresent... code was being executed at the end of the method regardless of whether there was a cache or not.

So you rolled back to the previous history via revert.

But this revert job seems to be rolling back this merged PR.

So I revert again, openAPIService.setCachedOpenAPI(openAPI, finalLocale); It has been modified to call the following two lines of code just before calling.

openApiLocaleCustomizers.values().forEach(openApiLocaleCustomizer -> openApiLocaleCustomizer.customise(openAPI, finalLocale));
openApiCustomisers.ifPresent(apiCustomizers -> apiCustomizers.forEach(openApiCustomizer -> openApiCustomizer.customise(openAPI)));

With this modification, I thought that it would be possible to handle duplicate header values ​​while keeping the previously merged PR changes.

The problem now depends on the order in which the test code performs the 3 lines below.

openAPIService.updateServers(openAPI);
openApiLocaleCustomizers.values().forEach(openApiLocaleCustomizer -> openApiLocaleCustomizer.customise(openAPI, finalLocale));
openApiCustomisers.ifPresent(apiCustomizers -> apiCustomizers.forEach(openApiCustomizer -> openApiCustomizer.customise(openAPI)));

I tried debugging SpringDocApp162Test.testApp2().

01

02

As shown in the screenshot, if openApiCustomisers.ifPresent(...) is called after calling updateServers(), it is overwritten with the OpenApiCustomiser bean set in the test code and the bean Servers are reflected in OpenAPI.

However, if called in the reverse order, Servers are emptied and default Server information is added by updateServer().

So, to catch the reflected PR contents and the duplicate header generation bug at the same time, it seems that the test code and environment modification are necessary.

If it is possible to modify the test code, I will work on it. If that is impossible or dangerous, it seems safe to revert as before. Please comment if I'm misunderstood 🙂

[bnasslahsen]

@doljae,

Preferably, all tests should still be passing without modification.
The business logic in AbstractOpenApiResource could be modified.

[doljae]

@bnasslahsen ,
I tried to improve by modifying only the logic of AbstractOpenApiResource without modifying the test code and data. But it was difficult, and the result is similar to the result you reverted.

As mentioned in the previous comment, the work you reverted is (probably) that the header is not duplicated every time you refresh the browser, but the merged PR work is rolled back.

As a result, to improve the previously merged PR and duplicate header issues at the same time, it seems that the test-related code and resources need to be modified.
Please let me know if there's a good way 🙏

By the way, could you please tell me why this test is failing? I'm not sure even if I check it through the debugger. The logic of testApp2() is to call testApp(), and since testApp()succeeded, shouldn't testApp2() also succeed?

[ahmedalnuaimi]

I think that you are cluttering the PR with final keywords and formatting. It is very hard to understand what changes did you make exactly due to that. Maybe add the minimum amount of changes to make it clear what are you trying to achieve here.
And regarding local final variables http://www.romanenco.com/final-local-variables-in-java.some-intentions-matter#:~:text=Actually%2C%20in%20java%2C%20there%20is,variable%20as%20a%20final%20one.

[doljae]

@ahmedalnuaimi , Thanks for the link.

First of all, talking about this PR, this PR hasn't been sorted out for a very long time now.

I left a comment to the maintainer by attaching materials such as screenshots about some test case modifications, but I haven't received any response since.

It has been quite a while since the new version was distributed by reverting the modifications previously uploaded to this project as PR.

Assuming that the existing test code is correct, it will be difficult for me to proceed with this related work, so I will close this PR.