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

Can ui-router handle periods in url query parameter names? #2395

Closed
SmilingJoe opened this issue Nov 25, 2015 · 6 comments
Closed

Can ui-router handle periods in url query parameter names? #2395

SmilingJoe opened this issue Nov 25, 2015 · 6 comments
Labels

Comments

@SmilingJoe
Copy link

Using ui-router v0.2.15. Have a route in our AngularJS (v1.4.7) application setup as follows:

.state('routeState',
{
    url: '/path/segment?param.sub.a&param.sub.b&param.sub.c',
    ...
}

As I understand the protocol, having periods inside of url query parameter names is valid. i.e.;

?first.name=Bobo&last.name=Muffy

When I setup this route in Angular, I get an error because the regex appears to be stripping out (or splitting up) the name portion into segments. So it essentially sees 'param.sub.a' as just 'param', and then errors out stating that 'param.sub.b' is a duplicate of 'param.sub.a' (because it only reads the 'param' portion of 'param.sub.b').

Unfortunately, we cannot change the parameter names as they are part of an existing protocol.

Is there a workaround to this issue? Hoping that I'm just missing something obvious already existing in the framework that can resolve this problem.

@SmilingJoe
Copy link
Author

After educating myself some on regular expressions, and then tracing the execution path of ui-router, I think I've narrowed down where this is occurring in the code, along with a possible fix.

Inside of angular-ui-router.js (v0.2.15), in the file urlMatcherFactory.js the variable searchPlaceholder is defined as follows:

searchPlaceholder = /([:]?)([\w\[\]-]+)|\{([\w\[\]-]+)(?:\:((?:[^{}\\]+|\\.|\{(?:[^{}\\]+|\\.)*\})+))?\}/g

Lower down is where any search parameters get processed by the following section of code:

if (search.length > 0) {
    last = 0;
    while ((m = searchPlaceholder.exec(search))) {
      p = matchDetails(m, true);
      param = addParameter(p.id, p.type, p.cfg, "search");
      last = placeholder.lastIndex;
      // check if ?&
    }
}

If I replace the \w\[\]- segments in the regex with \w\[\].- then it appears that periods are now considered as valid parts of the parameter name. This was attempted after researching what characters are actually considered to be valid for URI query parameters both here and here.

The final modified regular expression that seems to include periods as valid characters is:

searchParser = /([:]?)([\w\[\].-]+)|\{([\w\[\].-]+)(?:\:((?:[^{}\\]+|\\.|\{(?:[^{}\\]+|\\.)*\})+))?\}/g

Now I'm far from an expert in regular expressions, so there may be a better way to resolve that. As well, there might be another way altogether that is already part of the framework that I'm just not seeing. If so, please let me know.

@eddiemonge
Copy link
Contributor

Is the behavior the same if you define the params in a param: {} block instead of in the URL?

@eddiemonge
Copy link
Contributor

Also, can you write a failing/passing test for this?

@eddiemonge eddiemonge added the bug label Dec 3, 2015
@SmilingJoe
Copy link
Author

Hello,

Sorry for the delayed response.

So as far as using the params: {} object is concerned, correct me if I'm wrong, but isn't that used to specify default values for optional parameters?

I will write up the tests tomorrow for the proposed update I have.

@SmilingJoe
Copy link
Author

Hello Eddie,

Here is the test coverage I've added to the urlMatcherFactorySpec.js file. After running grunt against the latest modified repo, I see that 316 test complete successfully. Is this sufficient?

describe("parameters containing periods", function() {
    it("should match if properly formatted", function() {
        var matcher = new UrlMatcher('/users/?from&to&with.periods&with.periods.also');
        var params = matcher.parameters();

        expect(params.length).toBe(4);
        expect(params).toContain('from');
        expect(params).toContain('to');
        expect(params).toContain('with.periods');
        expect(params).toContain('with.periods.also');
    });

    it("should not match if invalid", function() {
        var err = "Invalid parameter name '.periods' in pattern '/users/?from&to&.periods'";
        expect(function() { new UrlMatcher('/users/?from&to&.periods'); }).toThrow(err);

        err = "Invalid parameter name 'periods.' in pattern '/users/?from&to&periods.'";
        expect(function() { new UrlMatcher('/users/?from&to&periods.'); }).toThrow(err);
    });
});

SmilingJoe pushed a commit to SmilingJoe/ui-router that referenced this issue Dec 9, 2015
…ontain periods

This change modifies the searchPlaceholder regex to include the period
character as valid for url query param names. This change also modifies
the regex found in the addParameter function which validates the param
name just prior to adding to the list of parameters. A new set of unit
tests were added to urlMatcherFactorySpec to ensure this modification
works correctly.

This change resolves issue angular-ui#2395
SmilingJoe pushed a commit to SmilingJoe/ui-router that referenced this issue Dec 14, 2015
…ontain periods

This change modifies the searchPlaceholder regex to include the period
character as valid for url query param names. This change also modifies
the regex found in the addParameter function which validates the param
name just prior to adding to the list of parameters. A new set of unit
tests were added to urlMatcherFactorySpec to ensure this modification
works correctly.

This change resolves issue angular-ui#2395
nateabele pushed a commit that referenced this issue Dec 15, 2015
…ontain periods

This change modifies the searchPlaceholder regex to include the period
character as valid for url query param names. This change also modifies
the regex found in the addParameter function which validates the param
name just prior to adding to the list of parameters. A new set of unit
tests were added to urlMatcherFactorySpec to ensure this modification
works correctly.

This change resolves issue #2395
@christopherthielen
Copy link
Contributor

Looks like this got resolved in d31b333

ExpFront pushed a commit to ExpFront/ui-router that referenced this issue Jun 23, 2016
…ontain periods

This change modifies the searchPlaceholder regex to include the period
character as valid for url query param names. This change also modifies
the regex found in the addParameter function which validates the param
name just prior to adding to the list of parameters. A new set of unit
tests were added to urlMatcherFactorySpec to ensure this modification
works correctly.

This change resolves issue angular-ui#2395
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants