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

Wrong values in generated config in case of root '/' #73

Closed
yildiz-online opened this issue Nov 15, 2019 · 8 comments
Assignees
Labels
bug

Comments

@yildiz-online
Copy link

@yildiz-online yildiz-online commented Nov 15, 2019

Hello, i noticed that running the configuration build from linux root result in wrong configuration output.

To reproduce: on linux, run this from the root directory:

`
private final static String SOURCE = "/aFile";

private final static String TARGET = "${user.dir}/lib/test/aFile";

public static void main(String[] args)  {
    Configuration config = Configuration
            .builder()
            .launcher("aMainClass")
            .baseUri("http://somewhere")
            .file(FileMetadata
                    .readFrom(Path.of(SOURCE))
                    .path(TARGET)
                    .classpath(false))
             )
            .build();
    try (Writer out = Files.newBufferedWriter(Path.of("wrong-update.xml"))) {
        config.write(out);
    } catch (Exception e) {
        e.printStackTrace();
    }
}`

In the resulting xml, the target "${user.dir}/lib/test/aFile"; will become "${user.dir}/lib/test${user.dir}aFile";

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 15, 2019

Good catch. Interestingly enough, according to the logic it should've replaced all slashes, not just the last.

Will investigate this next week.

@mordechaim mordechaim added the bug label Nov 15, 2019
@mordechaim mordechaim self-assigned this Nov 15, 2019
@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 15, 2019

A simple quick fix would be to add the following line to the builder chain:

.matchAndReplace(PlaceholderMatchType.NONE)
@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 15, 2019

This is very tricky. What if somebody really wants their slashes replaced (by, say, ${file.separator} which they set in an OS dependant property)?

I could hardcode a check if you're on Linux and root but I truly hate it.

@yildiz-online

This comment has been minimized.

Copy link
Author

@yildiz-online yildiz-online commented Nov 15, 2019

thx,

for me it is fine, my config is built in a CI and i put the app in a subfolder, this will do the trick for the time being, so take your time to find the best solution

@yildiz-online

This comment has been minimized.

Copy link
Author

@yildiz-online yildiz-online commented Nov 16, 2019

I am not sure the case will happen only in case of root /

i've not tested it but what if the the starting directory is /test, and the value of path is ${user.dir}/test/

will it not become ${user.dir}${user.dir}/ ? if this assumption is true you can just check the computed value of ${user.dir} is not part of the path (or uri, it is also affected)

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 17, 2019

The optimal approach would be to only match ${user.dir} to the beginning of the path.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 21, 2019

Detecting if user.dir is not enough, I have to accept file: prefix too. The tricky part is if the prefix itself was already replaced with a placeholder.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Nov 21, 2019

Then, the prefix might have any amount of slashes after the colon. This makes it extremely hard to distinguish between the scheme part and the root.

@mordechaim mordechaim closed this in bd11cd4 Dec 2, 2019
mordechaim added a commit that referenced this issue Dec 2, 2019
mordechaim added a commit that referenced this issue Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.