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

multiple _sort parameters can result in out of order SortSpec chain #371

Closed
euz1e4r opened this Issue May 24, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@euz1e4r

euz1e4r commented May 24, 2016

There a bug is the area of a restful FHIR query with multiple sort keys with various sort orders. The sort keys can come out in the wrong order when translated into the SortSpec linked list. HttpServletRequest and RestfulServer hold are held internally in a map by parameter name. The map makes no order guarantee. SortParameter and Constants specify three different parameters {_sort, _sort:asc, _sort:desc}. For a single parameter name used multiple times, the API delivers its values as an array with guaranteed order preservation.

If I specify exactly one sort key, the bug doesn't byte.
If I use exactly one of the parameter names above, the bug doesn't bite.
It requires a mix of the 3 parameter names to open up the possibility that they are delivered in the wrong order.

I've written a work around sort algorithm, which accepts the leading minus sign syntax, and we use only the _sort parameter and never _sort:asc or _sort:desc.

We were inspired by an version of the spec which used this leading minus sign syntax to mark descending sort keys. The more recent specification version introduces the :asc and :desc parameter modifier idea.

Trestful URL could look like ..._sort=key1&_sort=-key2&_sort=key3. I suggest adhering to and reverting to this earlier sort spec.

This moves the sort order specification from the key to the value. Also in this model we define exactly one parameter name, namely "_sort".

I've written and unit tested a possible modification of the method in question, below.

jar version: hapi-fhir-base-1.5.jar

method: SortParameter.translateQueryParametersIntoServerArgument(RequestDetails, BaseMethodBinding<?>)

public Object translateQueryParametersIntoServerArgument(RequestDetails theRequest, BaseMethodBinding<?> theMethodBinding) throws InternalErrorException, InvalidRequestException {
    String[] values = theRequest.getParameters().get(Constants.PARAM_SORT);
    if (values == null) {
        return null;
    }
    SortSpec headSpec = null;
    SortSpec tailSpec = null;
    SortOrderEnum order = null;
    for (String nextValue : values) {
        if (isNotBlank(nextValue)) {
            if (nextValue.startsWith("-")) {
                order = SortOrderEnum.DESC;
                nextValue = nextValue.substring(1);
                if (isBlank(nextValue)) {
                    continue;
                }
            }
            else {
                order = SortOrderEnum.ASC;
            }
            SortSpec spec = new SortSpec().setOrder(order).setParamName(nextValue);
            if (tailSpec == null) {
                headSpec = spec;
                tailSpec = spec;
            } else {
                tailSpec.setChain(spec);
                tailSpec = spec;
            }
        }
    }
    return headSpec;
}

@euz1e4r euz1e4r changed the title from bug with multiple sort parameters to multiple _sort parameters can result in out of order SortSpec chain May 24, 2016

@lawley

This comment has been minimized.

Contributor

lawley commented May 25, 2016

There are many web frameworks that do not make order guarantees about repeated parameters, so it's unwise for any RESTful API to place any significant on parameter order in the URL.

But I'm confused by your references to earlier and later specs and the _sort syntax.

1.0.2 (DSTU2) uses the :asc and :desc approach (which has the ordering issue), while 1.4.0 (and current build) uses the comma-separated list with leading minus signs.

1.0.2 https://www.hl7.org/fhir/search.html#sort
1.4.0 http://hl7.org/fhir/2016May/search.html#sort

@euz1e4r

This comment has been minimized.

euz1e4r commented May 25, 2016

Michael,

Thank you for your response.
In 1.5 Constants.java, the _sort, _sort:asc and _sort:desc parameters are still in the code and SortParameter.translateClientArgumentIntoQueryArgument parses them into a SortSpec list.
If one passes a comma separated list with minus signs, HAPI passes though as if it were a single sort key.
So as of 1.5, the implementation is lagging behind the spec. I can work around that.
Also, we've added the notion of paths to the sort keys, as in /Patient?_sort=name.family,name.given
We also support lists, which are quite common in DSTU. When we encounter a list, we sort by the elements of that list, stepping through in parallel.
I sort strings case insensitively, also.

    Best,
    Leonard

From: Michael Lawley [notifications@github.com]
Sent: Tuesday, May 24, 2016 5:28 PM
To: jamesagnew/hapi-fhir
Cc: Edmondson, Leonard; Author
Subject: Re: [jamesagnew/hapi-fhir] multiple _sort parameters can result in out of order SortSpec chain (#371)

There are many web frameworks that do not make order guarantees about repeated parameters, so it's unwise for any RESTful API to place any significant on parameter order in the URL.

But I'm confused by your references to earlier and later specs and the _sort syntax.

1.0.2 (DSTU2) uses the :asc and :desc approach (which has the ordering issue), while 1.4.0 (and current build) uses the comma-separated list with leading minus signs.

1.0.2 https://www.hl7.org/fhir/search.html#sorthttp://cp.mcafee.com/d/5fHCNESyMUMyOVtUQsCPqaaqbParXW9J55d5VBdNB4SrdCRr5vFiIendLWCCaEA4m1kwYwC_V1cl88czxlxf_5ny11MlKOxqSLII6zBBwSrdCPpesRG9pxjypUXw0eeAVhODX6Nfws1BaNCrdEThjdCXCQPrNKVJUSyrh
1.4.0 http://hl7.org/fhir/2016May/search.html#sorthttp://cp.mcafee.com/d/1jWVIg6h8SyMUMyOVtUQsCPqaaqbParXW9J55d5VBdNB4SrdCRr5vFiIendLWCCaEA4m1kwYwC_V1cl88czxlxf_5ny11MlKOxqSLII6zBBwSrdCPo0cCueU03zFeksjKyUdWmODX6Nfws1BaNCrdEThjdCXCQPrNKVJUSyrh


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHubhttp://cp.mcafee.com/d/2DRPoQcygQcCQm764mnbL6zASrhhjhupjvvhdEEFELcFKcECPpISHoHZalxOVJ_kQNl4wyMaA7A4T_89yF11AsaI9_UGYg8e2JSkbmRZBwQsII6PpISr9PCJhbc8OCjr8sLwzGLQQz3P64ZmAnt9OzAGkr-CczAgcBizvHG_2ptdd5xx55csrdEThjdCXCQPrNKVJUSyrh

@euz1e4r

This comment has been minimized.

euz1e4r commented May 25, 2016

I contribute this proposed implementation of the 1.4 _sort spec, in SortParameter.java, supporting the comma separated key list with optional leading minus for each key.

private static final Pattern comma = Pattern.compile(",", Pattern.LITERAL);
public Object translateQueryParametersIntoServerArgument(RequestDetails theRequest, BaseMethodBinding<?> theMethodBinding) throws InternalErrorException, InvalidRequestException {
    String[] values = theRequest.getParameters().get(Constants.PARAM_SORT);
    if (values == null) {
        return null;
    }
    if (values.length > 1) {
        throw new UnsupportedOperationException("at most one " + Constants.PARAM_SORT + " parameter is allowed");
    }
    SortSpec headSpec = null;
    SortSpec tailSpec = null;
    for (String nextValue : comma.split(values[0], 0)) {
        SortOrderEnum order = SortOrderEnum.ASC;
        if (isNotBlank(nextValue)) {
            if (nextValue.startsWith("-") && nextValue.length() > 1) {
                order = SortOrderEnum.DESC;
                nextValue = nextValue.substring(1);
            }
            SortSpec spec = new SortSpec().setOrder(order).setParamName(nextValue);
            if (tailSpec == null) {
                headSpec = spec;
                tailSpec = spec;
            } else {
                tailSpec.setChain(spec);
                tailSpec = spec;
            }
        }
    }
    return headSpec;
}

From: Michael Lawley [notifications@github.com]
Sent: Tuesday, May 24, 2016 5:28 PM
To: jamesagnew/hapi-fhir
Cc: Edmondson, Leonard; Author
Subject: Re: [jamesagnew/hapi-fhir] multiple _sort parameters can result in out of order SortSpec chain (#371)

There are many web frameworks that do not make order guarantees about repeated parameters, so it's unwise for any RESTful API to place any significant on parameter order in the URL.

But I'm confused by your references to earlier and later specs and the _sort syntax.

1.0.2 (DSTU2) uses the :asc and :desc approach (which has the ordering issue), while 1.4.0 (and current build) uses the comma-separated list with leading minus signs.

1.0.2 https://www.hl7.org/fhir/search.html#sorthttp://cp.mcafee.com/d/5fHCNESyMUMyOVtUQsCPqaaqbParXW9J55d5VBdNB4SrdCRr5vFiIendLWCCaEA4m1kwYwC_V1cl88czxlxf_5ny11MlKOxqSLII6zBBwSrdCPpesRG9pxjypUXw0eeAVhODX6Nfws1BaNCrdEThjdCXCQPrNKVJUSyrh
1.4.0 http://hl7.org/fhir/2016May/search.html#sorthttp://cp.mcafee.com/d/1jWVIg6h8SyMUMyOVtUQsCPqaaqbParXW9J55d5VBdNB4SrdCRr5vFiIendLWCCaEA4m1kwYwC_V1cl88czxlxf_5ny11MlKOxqSLII6zBBwSrdCPo0cCueU03zFeksjKyUdWmODX6Nfws1BaNCrdEThjdCXCQPrNKVJUSyrh


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHubhttp://cp.mcafee.com/d/2DRPoQcygQcCQm764mnbL6zASrhhjhupjvvhdEEFELcFKcECPpISHoHZalxOVJ_kQNl4wyMaA7A4T_89yF11AsaI9_UGYg8e2JSkbmRZBwQsII6PpISr9PCJhbc8OCjr8sLwzGLQQz3P64ZmAnt9OzAGkr-CczAgcBizvHG_2ptdd5xx55csrdEThjdCXCQPrNKVJUSyrh

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Jun 3, 2016

Hi All,

Thanks for reporting and for the detailed analysis. I've worked the attached patch in and made equivalent changes on the client side.

For now I've set it up so that the server will allow both DSTU2 and STU3 style sort params in order to allow people to migrate. Maybe this could be disabled later....

Will be checking in a patch shortly

@jamesagnew jamesagnew closed this in d966190 Jun 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment