Probable bug: method that nullifies model

[cstamas]

As discovered by @desruisseaux , this method is once used for "change detection" (if result is not null, apply change), and once directly where it -- probably by mistake -- nullifies the field value.

@gnodet

[gnodet]

I think the above line should be removed.

[gnodet]

This check should be changed to if (!Objects.equals(newDir, resource.getDirectory())

[gnodet]

Remove this new method.

[gnodet]

Originally, the code had the following:

    private String alignToBaseDirectory(String path, Path basedir) {
        return pathTranslator.alignToBaseDirectory(path, basedir);
    }

without the null return value.
We should restore this behavior and inline the equality check into the caller method.

[desruisseaux]

A slight amendment is to modify the method as below (with null return value replaced by the old value). It may allow to reuse more existing objects compared to inline equality check in alignToBaseDirectory(Resource), because alignToBaseDirectory(String) is also invoked directly elsewhere.

/**
 * Returns a path relocated to the given base directory. If the result of this operation
 * is the same path as before, then this method returns the old {@code path} instance.
 * It is okay for the caller to compare the {@link String} instances using the identity
 * comparator for detecting changes.
 *
 * @param path the path to relocate, or {@code null}
 * @param basedir the new base directory
 * @return relocated path, or {@code null} if the given path was null
 */
private String alignToBaseDirectory(String path, Path basedir) {
    String newPath = pathTranslator.alignToBaseDirectory(path, basedir);
    return Objects.equals(path, newPath) ? path : newPath;
}

And replace null check by identity check:

/**
 * Returns a resource with all properties identical to the given resource, except the paths
 * which are resolved according the given {@code basedir}. If the paths are unchanged, then
 * this method returns the previous instance.
 *
 * @param resource the resource to relocate, or {@code null}
 * @param basedir the new base directory
 * @return relocated resource, or {@code null} if the given resource was null
 */
@SuppressWarnings("StringEquality") // Identity comparison is ok in this method.
private Resource alignToBaseDirectory(Resource resource, Path basedir) {
    if (resource != null) {
        String oldDir = resource.getDirectory();
        String newDir = alignToBaseDirectory(oldDir, basedir);
        if (newDir != oldDir) {
            return resource.withDirectory(newDir);
        }
    }
    return resource;
}

[gnodet]

A slight amendment is to modify the method as below (with null return value replaced by the old value). It may allow to reuse more existing objects compared to inline equality check in alignToBaseDirectory(Resource), because alignToBaseDirectory(String) is also invoked directly elsewhere.

/**
 * Returns a path relocated to the given base directory. If the result of this operation
 * is the same path as before, then this method returns the old {@code path} instance.
 * It is okay for the caller to compare the {@link String} instances using the identity
 * comparator for detecting changes.
 *
 * @param path the path to relocate, or {@code null}
 * @param basedir the new base directory
 * @return relocated path, or {@code null} if the given path was null
 */
private String alignToBaseDirectory(String path, Path basedir) {
    String newPath = pathTranslator.alignToBaseDirectory(path, basedir);
    return Objects.equals(path, newPath) ? path : newPath;
}

And replace null check by identity check:

/**
 * Returns a resource with all properties identical to the given resource, except the paths
 * which are resolved according the given {@code basedir}. If the paths are unchanged, then
 * this method returns the previous instance.
 *
 * @param resource the resource to relocate, or {@code null}
 * @param basedir the new base directory
 * @return relocated resource, or {@code null} if the given resource was null
 */
@SuppressWarnings("StringEquality") // Identity comparison is ok in this method.
private Resource alignToBaseDirectory(Resource resource, Path basedir) {
    if (resource != null) {
        String oldDir = resource.getDirectory();
        String newDir = alignToBaseDirectory(oldDir, basedir);
        if (newDir != oldDir) {
            return resource.withDirectory(newDir);
        }
    }
    return resource;
}

Good point. In order to optimise things further, I think the above function needs to return null when nothing has changed. Or the map method could be adapted to check for object identity equality rather than null.

[desruisseaux]

In order to optimise things further, I think the above function needs to return null when nothing has changed. Or the map method could be adapted to check for object identity equality rather than null.

I was thinking to cascade the identity checks, as map is used not only for Resource.

[gnodet]

LGTM
@desruisseaux do you want to add anything to this PR ?

[desruisseaux]

Nothing to add. Thanks