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.

Sign up for GitHub

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

UI: [BUGFIX] Cope with service names that contain slashes #4756

Merged
merged 1 commit into from
Oct 11, 2018
Merged

UI: [BUGFIX] Cope with service names that contain slashes #4756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions ui-v2/app/adapters/service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
import Adapter from './application';
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/service';
import { OK as HTTP_OK } from 'consul-ui/utils/http/status';
const URL_PREFIX_SINGLE = 'health/service';
const URL_PREFIX_MULTIPLE = 'internal/ui/services';
export default Adapter.extend({
urlForQuery: function(query, modelName) {
return this.appendURL('internal/ui/services', [], this.cleanQuery(query));
return this.appendURL(URL_PREFIX_MULTIPLE, [], this.cleanQuery(query));
},
urlForQueryRecord: function(query, modelName) {
if (typeof query.id === 'undefined') {
throw new Error('You must specify an id');
}
return this.appendURL('health/service', [query.id], this.cleanQuery(query));
return this.appendURL(URL_PREFIX_SINGLE, [query.id], this.cleanQuery(query));
},
isQueryRecord: function(url, method) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you overriding an adapter method here? Curious why this new function wasn't called and couldn't find it in the adapter source after a quick look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it overrides a 'generic' method in the ApplicationAdapter. The ApplicationAdapter has a generic assumption for knowing whether a URL was to query a record (as oppose to query records). This Adapter inherits from that and therefore previously used the 'generic' isQueryRecord, using this 'generic' method was the root of this bug.

The 'generic' assumption was that the service name was always the last 'part' of the URL (so /health/service/service-name). That was the wrong assumption to make :) and the root of this bug, as people could have services with slashes in them meaning the last 'part' is no longer the full service name.

Curious why this new function wasn't called and couldn't find it in the adapter source after a quick look.

It's down in the handleResponse method

consul/ui-v2/app/adapters/service.js

Lines 23 to 37 in d8c9ab3

handleResponse: function(status, headers, payload, requestData) {
let response = payload;
const method = requestData.method;
if (status === HTTP_OK) {
const url = this.parseURL(requestData.url);
switch (true) {
case this.isQueryRecord(url, method):
response = this.handleSingleResponse(url, { Nodes: response }, PRIMARY_KEY, SLUG_KEY);
break;
default:
response = this.handleBatchResponse(url, response, PRIMARY_KEY, SLUG_KEY);
}
}
return this._super(status, headers, response, requestData);
},

// services can have slashes in them
// so just check for the prefix which is
// unique to the url here
const parts = url.pathname.split('/');
return `${parts[2]}/${parts[3]}` === URL_PREFIX_SINGLE;
},
handleResponse: function(status, headers, payload, requestData) {
let response = payload;
Expand Down
2 changes: 1 addition & 1 deletion ui-v2/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
]
},
"devDependencies": {
"@hashicorp/consul-api-double": "^1.4.0",
"@hashicorp/consul-api-double": "^1.5.3",
"@hashicorp/ember-cli-api-double": "^1.3.0",
"babel-plugin-transform-object-rest-spread": "^6.26.0",
"base64-js": "^1.3.0",
Expand Down
21 changes: 21 additions & 0 deletions ui-v2/tests/acceptance/dc/services/show-with-slashes.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
@setupApplicationTest
Feature: dc / services / show-with-slashes: Show Service that has slashes in its name
In order to view services that have slashes in their name
As a user
I want to view the service in the service listing and click on it to see the service detail
Scenario: Given a service with slashes in its name
Given 1 datacenter model with the value "dc1"
And 1 node model
And 1 service model from yaml
---
- Name: hashicorp/service/service-0
---
When I visit the services page for yaml
---
dc: dc1
---
Then the url should be /dc1/services
Then I see 1 service model
And I click service on the services
Then the url should be /dc1/services/hashicorp/service/service-0

10 changes: 10 additions & 0 deletions ui-v2/tests/acceptance/steps/dc/services/show-with-slashes-steps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import steps from '../../steps';

// step definitions that are shared between features should be moved to the
// tests/acceptance/steps/steps.js file

export default function(assert) {
return steps(assert).then('I should find a file', function() {
assert.ok(true, this.step);
});
}
6 changes: 3 additions & 3 deletions ui-v2/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@
faker "^4.1.0"
js-yaml "^3.10.0"

"@hashicorp/consul-api-double@^1.4.0":
version "1.5.1"
resolved "https://registry.yarnpkg.com/@hashicorp/consul-api-double/-/consul-api-double-1.5.1.tgz#73ce7696dc4475f69a59462e6690611b73ec6ced"
"@hashicorp/consul-api-double@^1.5.3":
version "1.5.3"
resolved "https://registry.yarnpkg.com/@hashicorp/consul-api-double/-/consul-api-double-1.5.3.tgz#c797bd702c1c1f9c669b9df7f95126b3a76dd1c6"

"@hashicorp/ember-cli-api-double@^1.3.0":
version "1.6.1"
Expand Down