Multiple swagger tags not expanding issue 6996

[rhwilburn]

Description

Motivation and Context

When you use more than one swagger UI instance in a single webpage and require operations to be expanded by default, the instances fight against each other, resulting only the first swagger UI HTML instance rendering and the other instances spin loading until manually toggling the operations.

This fixes issue: #6996

Fixes #6996

How Has This Been Tested?

I have manually tested by using 3 swagger UI specs (all locally hosted; as they seem to give the problem much worse than public internet URLs as delays in the internet will reduce race conditions that reproduce this issue.

Specifically I updated dev-helper-initializer.js and index.html under the dev-helpers folder and used npm run dev command

I have tried some external URLs for swagger spec like Petshop example.

I have tested on Windows using Chrome

I did two weeks of research to solve this issue; despite the small amount of code change. I had originally wanted to use reducers.js but found that the changes were getting large and other problems were being introduced. Due to my non familiarity with Redux, I didn't want to take on a big refactoring, so the changes have been limited to a single file.

Screenshots (if appropriate):

Screenshots of the issue are in the issue mentioned previously.

Checklist

My PR contains...

• No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
• Dependency changes (any modification to dependencies in package.json)
• Bug fixes (non-breaking change which fixes an issue)
• Improvements (misc. changes to existing features)
• Features (non-breaking change which adds functionality)

My changes...

• are breaking changes to a public API (config options, System API, major UI change, etc).
• are breaking changes to a private API (Redux, component props, utility functions, etc.).
• are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
• are not breaking changes.

Documentation

• My changes do not require a change to the project documentation.
• My changes require a change to the project documentation.
• If yes to above: I have updated the documentation accordingly.

Automated tests

• My changes can not or do not need to be tested.
• My changes can and should be tested by unit and/or integration tests.
• If yes to above: I have added tests to cover my changes.
• If yes to above: I have taken care to cover edge cases in my tests.
• All new and existing tests passed.

[rhwilburn]

Note: You can check this out with: yarn add rhwilburn/swagger-ui#multiple-swagger-tags-not-expanding-issue-6996

[rhwilburn]

@mathis-m @char0n @tim-lai Appreciate there is a queue for PRs etc but just a note this is a very small change (github diff makes it look bigger; but its a single file and under 10 lines I think). Is there something I can do to help get this reviewed or approved etc.

[mathis-m]

@mathis-m @char0n @tim-lai Appreciate there is a queue for PRs etc but just a note this is a very small change (github diff makes it look bigger; but its a single file and under 10 lines I think). Is there something I can do to help get this reviewed or approved etc.

Hi there, thanks for your work, I had a brief look at it and LGTM. I am just a fellow contributor, so can not give a final review.

Maybe you could add tests if applicable, so if the maintainers will review this PR, you don’t have to do it then and wait for another review cycle :)

Cheers Mathis

[char0n]

@rhwilburn thanks for looking into this. Reviewing this is on my radar already. As this is the single most critical code for performing lazy resolution, I'll need to looks into this carefully.

[char0n]

Hi @rhwilburn,

Thanks again for the PR. I've studied the changes and I have couple of questions:

Q1

AFAICT specUrl is used as a discriminator for the system instances. What if the there are two SwaggerUI instances rendered on the page and the SwaggerUI was provided definition as spec option instead of url. In that case both SwaggerUI instances will return empty string "" when calling system.spec().get('url'). I am assuming that will break the algorithm right?

[char0n]

I would suggest the following:

requestResolvedSubtree is a thunk that receives system specific to the current SwaggerUI instance. Without changing the architecture in a significant way, we can change the requestResolvedSubtree to look like this:

export const requestResolvedSubtree = path => system => {
  // poor-man's array comparison
  // if this ever inadequate, this should be rewritten to use Im.List
  const isPathAlreadyBatched = requestBatch
    .map(arr => arr.join("@@"))
    .indexOf(path.join("@@")) > -1

  if(isPathAlreadyBatched) {
    return
  }

  requestBatch.push({ path, system })
  debResolveSubtrees()
}

Now the requestBatch is a list of objects containing path + system combo. When the debResolveSubtrees executes, it just need to compensate for the fact that every requestBatch item comes with the specific system instance and all calls to selectors and actions needs to be done on that specific system instance. And then at the end of the debResolveSubtrees, the requestBatch is assign en empty array again.

Would that work for you @rhwilburn?

[rhwilburn]

Hi @rhwilburn,

Thanks again for the PR. I've studied the changes and I have couple of questions:

Q1

AFAICT specUrl is used as a discriminator for the system instances. What if the there are two SwaggerUI instances rendered on the page and the SwaggerUI was provided definition as spec option instead of url. In that case both SwaggerUI instances will return empty string "" when calling system.spec().get('url'). I am assuming that will break the algorithm right?

Good catch; yes the url would be required and must be unique in my approach. I would agree its a clumbsy approach as it could be legitimate to have the same swagger spec display twice on a given HTML page for someone. It could also be as you say that the swagger url is empty twice which would also be an issue.

I think the changes you have proposed make sense so will update it with them and have a look.

[rhwilburn]

@char0n I have made the changes you suggested

[char0n]

@char0n I have made the changes you suggested

Hi @rhwilburn,

The changes you've introduced were unfortunately failing most of the test. I've pushed two additional commits to this PR which fix the issues.

Here is the list of the issues I was able to identify:

1

It's best to avoid modifying iterable (in this case array) while iterating it.

Referring to delete requestBatch[i]

2

By using delete operator, you're creating sparse array, which will cause issues further in the loop

Referring to delete requestBatch[i]

a = [1, 2]
a.lenght; // 2
delete a[1]
a // [1, empty]
a.length; // 2

3

isPathAlreadyBatched was not handled correctly. We have to take care of comparing
system as well. There can be two identical paths but specific to different system (SwaggerUI instance).

[char0n]

@rhwilburn thanks for opening this up and pushing for a workable solution. When the tests pass, I'll merge the PR.