The Scenario
So I was working away on an EF6 / WebAPI / AngularJS project when I ran into an odd issue. I had secured my WebAPI using OAuth and required that the user provide a bearer token on every request. The strategy / workflow worked like this:
1. User browses to https://myproject.domain.net/login
2. Angular’s route provider recognizes this route and tees up a loginController and renders the view from the loginTemplate.html
3. User provides username/password and clicks the “Login” button.
4. The loginController component leverages an injected loginService to submit the credentials to the WebAPI on the middle-tier.
5. The WebAPI attempts to authenticate the user and if successful, provides a bearer token in the response payload.
6. The loginService stashes the bearer token in local storage to be used with subsequent calls to WebAPI courtesy of a custom interceptor added to the $httpProvider which would lookup the available bearer token and inject it into each request.
7. Should this bearer token become invalid, the WebAPI would return a 401-Unauthorized for any attempt to call a method which required authorization, and the user would be shuttled back to the login page.
All was fine and dandy in the world and this worked well. However after playing with it a few times, I thought it would be nice to do the user a courtesy and take them back to the original page they requested that resulted in the 401 initially once they successfully re-authenticated. This required jamming a path onto my URL like this https://myproject.domain.net/login/<originally attempted uri>.
This was no problem and leveraging the ability to use wildcard route group parameters in Angular made it easy – I simply setup my routeProvider’s when condition to look for “/login/:destination*”. This way – I could capture the whole original path – which very likely contained slashes. A user could attempt to go look at /projects/4/sites/1, get a 401-Unauthorized back and be directed to /login/projects/4/sites/1 and be carried over to /projects/4/sites/1 easily after providing their login credentials.
The Problem
The issue I very quickly encountered was the users who navigated straight to /login from the home page of the site were met with a sharp return to the / URL … as though the request to /login didn’t match any of the defined routes in the routeProvider. However, a request to /login/ worked just fine. It became apparent to me at that moment that even though I was using a wildcard parameter on the /login/:destination* route – the parameter was still a required parameter, which resulted in the route requiring a trailing slash. Trailing slashes never look good, and trying to tell users that if they simply wanted to browse to the login page, they’d need to take care to include a trailing slash was a non-starter, and I really didn’t want to include two separate, but very similar-looking, route definitions for “/login” and “/login/:destination*”. I knew that Angular also provided a convention for optional parameters too by using the “?” character on the end of the parameter group name. So “/login/:destination?” would serve requests for both “/login” and “/login/<some destination>” … as long as that destination parameter group didn’t contain slashes. I tried using the syntax of “/login/:destination*?” to make the parameter group by wildcard AND optional – but to my dismay, Angular only recognized a single character on the end of the parameter group’s name. It could either be wildcard (*) or optional (?) but not both. The giant door of disappointment slammed shut. You can’t get there from here. Take a hike and get bent.
The Solution
After despondently staring into my coffee cup for a bit, I decided “to heck with it! I’ll just modify the Angular core .. which is ALWAYS a fantastic idea for portability and long-term maintainability! </sarcasm>” I dug into the angular-route.js file and found the pathRegExp function which tore apart the route path, looking for the two options for wildcard or optional parameter groups, and beat up on the regular expression that made the match. I refactored the code as such:
Original Code
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
|
path = path .replace(/([().])/g, '\\$1' ) .replace(/(\/)?:(\w+)([\?\*])?/g, function (_, slash, key, option) { var optional = option === '?' ? option : null ; var star = option === '*' ? option : null ; keys.push({ name: key, optional: !!optional }); slash = slash || '' ; return '' + (optional ? '' : slash) + '(?:' + (optional ? slash : '' ) + (star && '(.+?)' || '([^/]+)' ) + (optional || '' ) + ')' + (optional || '' ); }) .replace(/([\/$\*])/g, '\\$1' ); |
Refactored Code
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
|
path = path .replace(/([().])/g, '\\$1' ) .replace(/(\/)?:(\w+)(\*\?|[\?\*])?/g, function (_, slash, key, option) { var optional = (option === '?' || option === '*?' ) ? '?' : null ; var star = (option === '*' || option === '*?' ) ? '*' : null ; keys.push({ name: key, optional: !!optional }); slash = slash || '' ; return '' + (optional ? '' : slash) + '(?:' + (optional ? slash : '' ) + (star && '(.+?)' || '([^/]+)' ) + (optional || '' ) + ')' + (optional || '' ); }) .replace(/([\/$\*])/g, '\\$1' ); |
From careful examination of the highlighted expression in the refactored code above, you can see that it now also searches for “*?” on the end of a parameter group and sets the “optional” and “star” variables accordingly. This allows the route processor to create the necessary additional routes to handle the route with or without a trailing slash, and with a parameter group that might contain a ton of slashes, honoring the wildcard requirement.
Thinking that this was a pretty cool feature, and not wanting to be horribly selfish (or create a bunch of code that would always need to be maintained and updated with extreme care as to not overwrite my carefully applied custom patches, I decided to contribute to the AngularJS project on GitHub. With stars in my eyes and the hopes that the Angular gods would bless my modifications to their code, I began down the winding path of how to submit a pull request to the GitHub repository where AngularJS lives.
The Process (and the pain)
As with most things computer-programming related, the perception is that it’s easy. You just draw your boxes on the screen, and then it just works and you’re done, right? I thought – well, this is simple enough .. I’ll just fork the AngularJS repository over to my GitHub account, branch, clone, make my various mods and changes, commit, push and then submit a pull request. Easy as pie, right?
But then there are standards. And there are spaces. Sometimes too many of them. Especially when you’re switching between Windows and Unix environments, and tabs either becomes spaces or vice-versa.
I spent a little time looking through: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md. I wanted to make sure that I honored all the odd little requirements, e.g. “Wrap all code at 100 characters”
Once I finished my changes, authored my tests and made sure all looked well from my side, I ran through the Jasmine and Karma tests and then made my pull request. I signed the Contributor License Agreement and was delighted to watch TravisCI run through testing with green lights. Everything worked without issue! I was well on my way to becoming an AngularJS contributor and waited on bated breath for my changes to be accepted and merged
90 minutes later, I noticed some activity on my pull request. Could it be so soon? I read through the comments and was aghast to find that a couple of odd grunt requirements had snuck into my package.json file, and that the indentation in my routeSpec.js was off. With tears in my eyes, I went back to my project and corrected my spacing and grunt dependencies. I updated my pull request and began the process of waiting …and waiting …and waiting. For 5 days.
On the 5th day, I meagerly commented and stated that I believed I had satisfied all the various requirements and to please let me know if anything was outstanding.
The next day I received the following:
My feature had been deemed important enough that it was reclassified as a fix and backported to earlier versions of the framework. Victory!