Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the failed test due to hardcoded file separators #2366

Merged
merged 1 commit into from
Oct 15, 2023
Merged

Fix the failed test due to hardcoded file separators #2366

merged 1 commit into from
Oct 15, 2023

Conversation

wnineg
Copy link
Contributor

@wnineg wnineg commented Aug 31, 2023

This specific test fails under Windows: the actual path utilized File.separator but not the expected one.

@vojkog
Copy link

vojkog commented Sep 11, 2023

I stumbled upon the same issue; maybe we can, at the same time, avoid using hardcoded "version" in the expected String, e.g.?

 @Test
    void findWebJarResourcePath() {
        String path = "swagger-ui/swagger-initializer.js";

        String actual = abstractSwaggerResourceResolver.findWebJarResourcePath(path);
        String[] pathNames = {"swagger-ui", VERSION, "swagger-initializer.js"};
        String expected = String.join(File.separator, pathNames);

        assertEquals(expected, actual);
    }

@wnineg
Copy link
Contributor Author

wnineg commented Sep 11, 2023

Yeah...I knew I could go a little bit fancier using Paths.get (more semantics), but since I was lazy so I just fixed it using the same way of how actual was constructed. Quick change, all test passed, and I called it a day.

Now since you mentioned it, instead of using String.join, the assertion can be like:

assertEquals(Paths.get("swagger-ui" ,"4.18.2" ,"swagger-initializer.js").toString(), actual);

Additionally, I went back to see if I could clean it up more. So the method that constructs the actual path (org.springdoc.ui.AbstractSwaggerResourceResolver#findWebJarResourcePath) works like this:

  1. Invokes the private method webjar to extract the very first 'component' of the given path. (TBH the name of the method is a bit confusing as it has nothing to do with webjar(?) but just plain logic to extract a path component.)
  2. If there the first path component doesn't exist, or the component is the full path itself, return null.
  3. Otherwise, returns a path by adding the SwaggerUI version number between the first path component and the rest.

I can further clean up the logic there by utilizing java.nio.file.Path.

@wnineg
Copy link
Contributor Author

wnineg commented Sep 11, 2023

So, as my last comment mentioned, the implementation of org.springdoc.ui.AbstractSwaggerResourceResolver#findWebJarResourcePath can be refactored to:

	protected String findWebJarResourcePath(String pathStr) {
		Path path = Paths.get(pathStr);
		if (path.getNameCount() < 2) return null;
		String version = swaggerUiConfigProperties.getVersion();
		if (version == null) return null;
		Path first = path.getName(0);
		Path rest = path.subpath(1, path.getNameCount());
		return first.resolve(version).resolve(rest).toString();
	}

Which eliminates the only references to the private methods webjar(String) and path(String, String), enabling me to cleanup even more lines of code. Though, I wonder if I am doing too much. If the maintainers are cool with it, I can push the change.

@vojkog
Copy link

vojkog commented Sep 11, 2023

Thank you, @wnineg, for your answer.

I agree. I created a "fix" before I saw that you had already done it, so I just compared/commented solutions, also without going into details.
Maintainers will probably know best what is "good enough" to close this issue.

@bnasslahsen
Copy link
Collaborator

@wnineg and @vojkog,

Thank you for your contributions and interesting discussion.
Just merged your proposed changes.

@bnasslahsen bnasslahsen merged commit a41882d into springdoc:main Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants