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

UriTemplate expansion is inconsistent when using beans to pass the values #11192

Open
yawkat opened this issue Sep 18, 2024 Discussed in #11191 · 5 comments
Open

UriTemplate expansion is inconsistent when using beans to pass the values #11192

yawkat opened this issue Sep 18, 2024 Discussed in #11191 · 5 comments

Comments

@yawkat
Copy link
Member

yawkat commented Sep 18, 2024

Discussed in #11191

Originally posted by nedelva September 18, 2024
I was playing with UriTemplate and I wrote a JUnit 5 test to check if it implements all of the features shown in URI.js.

package io.micronaut.http.uri;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.converter.ArgumentConversionException;
import org.junit.jupiter.params.converter.ConvertWith;
import org.junit.jupiter.params.converter.SimpleArgumentConverter;
import org.junit.jupiter.params.provider.CsvSource;

import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class UriTemplateTest {

    private UriTemplate uriTemplate;

    @ParameterizedTest
    @CsvSource(delimiter = '|', value = {
            "/users{?id*} | 3,4,5 | /users?id=3&id=4&id=5", //style==form,spaceDelimited & explode==true
            "/users{?id} | 3,4,5 | /users?id=3,4,5",  //style==form & explode==false
            "/users{?id*} | 3,4,5 | /users?id=3&id=4&id=5" //style==pipeDelimited & explode==true
    })
    public void testQueryParams(String uriTemplate, @ConvertWith(StringToListConverter.class) List<String> inputList, String expectedExpansion) {
        this.uriTemplate = new UriTemplate(uriTemplate);
        String expansion = this.uriTemplate.expand(Map.of("id", inputList));
        assertEquals(expectedExpansion, expansion);
    }

    @ParameterizedTest
    @CsvSource(delimiter = '|', value = {
            "{+id*}|one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{;id*}|;one=alpha;two=bravo",
            "{&id*}|&one=alpha&two=bravo",
    })
    void testDeepObjectMapParam(String template, String expected) {
        uriTemplate = new UriTemplate(template);
        Map<String, String> map = new HashMap<>();
        map.put("one", "alpha");
        map.put("two", "bravo");

        assertEquals(expected, uriTemplate.expand(Map.of("id", map)));
    }

    @ParameterizedTest
    @CsvSource(delimiter = '|', value = {
            "{+id*}|one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{;id*}|;one=alpha;two=bravo",
            "{&id*}|&one=alpha&two=bravo",
    })
    void testDeepObjectParam(String template, String expected) {
        uriTemplate = new UriTemplate(template);
        DeepObject deepObject = new DeepObject("alpha", "bravo");
        String expansion = this.uriTemplate.expand(Map.of("id", deepObject));
        assertEquals(expected, expansion);

    }

    @ParameterizedTest
    @CsvSource(delimiter = '|', value = {
            "/users/{+id*} | 3,4,5 | /users/3,4,5", //style==simple & explode==true
            "/users/{+id} | 3,4,5 | /users/3,4,5",  //style==simple & explode==false
            "/users/{.id*} | 3,4,5 | /users/.3.4.5", //style==label & explode==true
            "/users/{.id} | 3,4,5 | /users/.3,4,5",  //style==label & explode==false
            "/users/{;id*} | 3,4,5 | /users/;id=3;id=4;id=5", //style==label & explode==true
            "/users/{;id} | 3,4,5 | /users/;id=3,4,5",  //style==label & explode==false
    })
    public void testPathParams(String uriTemplate, @ConvertWith(StringToListConverter.class) List<String> inputList, String expectedExpansion) {
        this.uriTemplate = new UriTemplate(uriTemplate);
        String expansion = this.uriTemplate.expand(Map.of("id", inputList));
        assertEquals(expectedExpansion, expansion);
    }

    static class StringToListConverter extends SimpleArgumentConverter {
        @Override
        protected Object convert(Object source, Class<?> targetType) throws ArgumentConversionException {
            if (source instanceof String && List.class.isAssignableFrom(targetType)) {
                String[] values = ((String) source).split(",");
                return Arrays.asList(values);
            } else {
                throw new ArgumentConversionException("Cannot convert source type " + source.getClass() + " to target type List");
            }
        }
    }
}

In the test testDeepObjectParam I am using a record defined as:

package io.micronaut.http.uri;
import io.micronaut.core.annotation.Introspected;

@Introspected
public record DeepObject(String one, String two) {
}

The test using DeepObject is only mimicking the testDeepObjectMapParam but using a record instead of a Map. The funny thing is it fails only a few cases but not always the same lines (in my runs, lines 1, 3, 5 or just 3 and 4 from the CsvSource).
The failure always is about the order of evaluation of the values in the expansion:

[3] template={#id*}, expected=#one=alpha,two=bravo : 
Expected :#one=alpha,two=bravo
Actual   :#two=bravo,one=alpha
[4] template={;id*}, expected=;one=alpha;two=bravo
Expected :;one=alpha;two=bravo
Actual   :;two=bravo;one=alpha

I also noticed that if the test method testDeepObjectParam is copied as the sole test in another test class, different cases fail (!)
(Perhaps some race condition is occurring?)

Shouldn't this expansion produce predictable results? (Not that it matters for the expanded URI)

Final note: the HashMap is evaluated in the same way, but this is a happy case. If I use Map.of("one","alpha","two","bravo") the test testDeepObjectMapParam will start failing as well a few cases.

@dstepanov
Copy link
Contributor

That class is replaced by UriTemplateMatcher

@nedelva
Copy link

nedelva commented Sep 18, 2024

That class is replaced by UriTemplateMatcher

I did not get this memo 😀 . Now that you mention it @dstepanov, I believe this intention should be properly documented (a mention in the Guide and the UriTemplate class marked as @Deprecated)

@nedelva
Copy link

nedelva commented Sep 18, 2024

Well, the class UriTemplateMatcher suffers from the same ailment as UriTemplate does.
Just run this test to see for yourself:

package io.micronaut.http.uri;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;

class UriTemplateMatcherTest {

    @ParameterizedTest
    @CsvSource(delimiter = '|', value = {
            "{+id*}|one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{#id*}|#one=alpha,two=bravo",
            "{;id*}|;one=alpha;two=bravo",
            "{&id*}|&one=alpha&two=bravo",
    })
    void testDeepObjectParam(String template, String expected) {
        UriTemplateMatcher templateMatcher = new UriTemplateMatcher(template);
        DeepObject deepObject = new DeepObject("alpha", "bravo");
        String expansion = templateMatcher.expand(Map.of("id", deepObject));
        assertEquals(expected, expansion);

    }
}

@dstepanov
Copy link
Contributor

I didn't deprecate it because it's used in the routes API, and there is no way to use the new one—the idea is to replace one with another in v5.
I think that in the future version (maybe even before v5), I would like to have an option for controllers to select what kind of URI style you want: current one based on uri template or simple JAX-RS style.

@nedelva
Copy link

nedelva commented Oct 23, 2024

@dstepanov, @yawkat Are there any plans to fix this bug?

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

No branches or pull requests

3 participants