[shabaz213]

List of changes

• Removed redundant quote in the toString()
• Mask credentials in toString()
• Normalize basePath to handle slashes
• Made setUser package-private
• Removed tests with invalidBasePathFailure as it is no longer throwing expected exception instead it is throwing ZosmfRequestException

[frankgiordano]

How about change this method as follows:

/**
 * Normalizes a path by collapsing multiple slashes, resolving relative segments (. and ..),  
 * and ensuring a consistent leading slash. The result is standardized to forward slashes   
 * and excludes a trailing slash unless it is the root path.  
 * 
 * @param basePath The raw path string to normalize.
 * @return A canonical Unix-style path string.
 */
private String getNormalizedPath(String basePath) {
    // Paths.get handles leading slashes and collapses internal duplicate slashes
    // normalize() resolves dot-segments like /./ and /../
    final String normalized = Paths.get("/", basePath).normalize().toString().replace("\\", "/");
    
    // Remove trailing slash unless the result is the root "/"
    return (normalized.length() > 1 && normalized.endsWith("/"))
            ? normalized.substring(0, normalized.length() - 1) : normalized;
}

[shabaz213]

We can replace all the backward slashes with forward slash in the same logic, but I don't think its a good idea to use "Paths" class as it will interpret "file separator" differently based on OS, for windows and Unix it will be different.

[shabaz213]

Pushed a new commit to replace backward slashes to forward slashes

[frankgiordano]

ok thanks.. with your updated version and without using Path how about we change the method to be like this way:

/**
 * Normalizes a path string by standardizing separators, collapsing multiple 
 * consecutive slashes, and ensuring a consistent leading slash.
 * <p>
 * This method performs the following transformations:
 * <ul>
 *   <li>Converts all backslashes (\) to forward slashes (/).</li>
 *   <li>Collapses multiple consecutive forward slashes into a single slash.</li>
 *   <li>Ensures the path begins with a leading slash.</li>
 *   <li>Removes the trailing slash unless the path is the root "/".</li>
 * </ul>
 *
 * @param basePath raw path string to be normalized.
 * @return canonicalize path string, or "/" if the input is null or blank.
 */
private String getNormalizedPath(String basePath) {
    // standardize separators and ensure a leading slash
    String normalizedPath = basePath.replace('\\', '/');
    normalizedPath = normalizedPath.startsWith("/") ? normalizedPath : "/" + normalizedPath;
    
    // collapse two or more consecutive slashes into one
    normalizedPath = normalizedPath.replaceAll("/{2,}", "/");

    // remove trailing slash unless the result is just the root "/"
    if (normalizedPath.length() > 1 && normalizedPath.endsWith("/")) {
        normalizedPath = normalizedPath.substring(0, normalizedPath.length() - 1);
    }

    return normalizedPath;
}

[frankgiordano]

actually lets change the beginning to this to handle spaces too:
// remove all spaces and standardize separators and ensure a leading slash
String path = basePath.replace(" ", "").replace('\', '/');

[frankgiordano]

Why do you feel this is redundant?

[shabaz213]

I couldn't able to get a "true" out of this method in unit testing.

[shabaz213]

Added it back, found a way to get "true" out of this method with some modification to host

[frankgiordano]

for this method how about we change it too:

/**
 * Sets the base path after normalizing it for z/OSMF REST endpoints. 
 * If the input is null or blank, it sets the value as is.
 * <p>
 * This method's access level is private-package
 *
 * @param basePath string value
 */
void setBasePath(String basePath) {
    this.basePath = Optional.ofNullable(basePath)
            .filter(path -> !path.isBlank())
            .map(this::getNormalizedPath)
            .orElse(basePath);
}

[shabaz213]

This looks like complicated code instead of simple if-else

[frankgiordano]

ok never mind.

[frankgiordano]

ok thanks.. with your updated version and without using Path how about we change the method to be like this way:

/**
 * Normalizes a path string by standardizing separators, collapsing multiple 
 * consecutive slashes, and ensuring a consistent leading slash.
 * <p>
 * This method performs the following transformations:
 * <ul>
 *   <li>Converts all backslashes (\) to forward slashes (/).</li>
 *   <li>Collapses multiple consecutive forward slashes into a single slash.</li>
 *   <li>Ensures the path begins with a leading slash.</li>
 *   <li>Removes the trailing slash unless the path is the root "/".</li>
 * </ul>
 *
 * @param basePath raw path string to be normalized.
 * @return canonicalize path string, or "/" if the input is null or blank.
 */
private String getNormalizedPath(String basePath) {
    // standardize separators and ensure a leading slash
    String normalizedPath = basePath.replace('\\', '/');
    normalizedPath = normalizedPath.startsWith("/") ? normalizedPath : "/" + normalizedPath;
    
    // collapse two or more consecutive slashes into one
    normalizedPath = normalizedPath.replaceAll("/{2,}", "/");

    // remove trailing slash unless the result is just the root "/"
    if (normalizedPath.length() > 1 && normalizedPath.endsWith("/")) {
        normalizedPath = normalizedPath.substring(0, normalizedPath.length() - 1);
    }

    return normalizedPath;
}

[frankgiordano]

This test does not consider base path. Tweak is as the following I suggest:

@Test
public void tstUrlConstructionWithInvalidHostNameFailure() {
    // Create a connection
    final ZosConnection connection = ZosConnectionFactory
            .createBasicConnection("test.host:123foo", "443", "user", "password");
    // Create a mock request to verify URL
    final ZosmfRequest request = ZosmfRequestFactory.buildRequest(connection, ZosmfRequestType.GET_JSON);

    // Set URL for a hypothetical endpoint
    final String RESOURCE_PATH = "/resource/endpoint";
    assertThrows(IllegalArgumentException.class,
            () -> request.setUrl(connection.getZosmfUrl() + RESOURCE_PATH));
}

[shabaz213]

renamed test

[shabaz213]

@frankgiordano I understand you want the project to have useful comments consistently but this is not required for a clean and readable code.

[frankgiordano]

can we do the following here:
String normalizedPath = basePath.replace(" ", "").replace('', '/');

[frankgiordano]

I meant to say:
String normalizedPath = basePath.replace(" ", "").replace('\', '/');

[frankgiordano]

I think here we can just use:
.createBasicConnection("test.host:123foo", "443", "user", "password");

[frankgiordano]

I think this is all good. I was over thinking on edge cases but it is not needed.