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

Buggy resource path resolution #52

Closed
rufuspollock opened this issue Jan 17, 2017 · 2 comments · Fixed by #48
Closed

Buggy resource path resolution #52

rufuspollock opened this issue Jan 17, 2017 · 2 comments · Fixed by #48
Assignees
Labels
Milestone

Comments

@rufuspollock
Copy link
Contributor

https://github.com/frictionlessdata/datapackage-js/blob/master/src/resource.js#L73

  get source() {
    if (this._sourceKey === 'path' && this._basePath) {
      const resourcePath = this.descriptor[this._sourceKey]
      return path.normalize(`${this._basePath}/${resourcePath}`)
    }

    return this.descriptor[this._sourceKey]
  }

This is not the right logic: we need to check whether resource path attribute is a fully qualified url or not before prepending the basepath - we only prepend base if path is not a fully qualified url.

Aside: as general code practice i think this should be an if / else statement not if then a return - the logic flow would be clearer.

Aside: type as a function seems a too generic name -- typeOfResourcePath would be a more informative name and would obviate much of the documentation ;-)

@roll
Copy link
Member

roll commented Jan 18, 2017

Thanks, it will be addressed here - #48 (cc @dumyan)

@roll roll added this to the Update milestone Jan 18, 2017
@roll roll unassigned dumyan Jan 18, 2017
@roll roll added the review label Jan 18, 2017
@rufuspollock
Copy link
Contributor Author

@roll another (show stopper bug) is that you do:

return path.normalize(`${this._basePath}/${resourcePath}`)

This leads to errors if your base path is a http url since // becomes / e.g. this will fail:

const url = 'https://raw.githubusercontent.com/frictionlessdata/dpr-js/gh-pages/fixtures/dp2/datapackage.json'
const basePath = 'https://raw.githubusercontent.com/frictionlessdata/dpr-js/gh-pages/fixtures/dp2'
const dp = await new Datapackage(url, 'base', true, false, basePath);

# this will fail
# expect https://raw.githubusercontent.com/frictionlessdata/dpr-js/gh-pages/fixtures/dp2/data/demo-resource.csv
# but get https:/raw.githubusercontent.com/frictionlessdata/dpr-js/gh-pages/fixtures/dp2/data/demo-resource.csv
# note single slash in https:/
expect(dp.resources[0].source).toEqual(basePath + '/data/demo-resource.csv')

I think this is fixed in #48

@roll roll closed this as completed in #48 Jan 24, 2017
@roll roll removed the review label Jan 24, 2017
roll pushed a commit that referenced this issue Jan 24, 2017
* Handle relative resource paths for URLs

* Add remote relative resource path test

* Don't prepend base path if the resource path is URL

* New datapackage for remote resource URL tests

* Make _getBasePath static

* Add _getBasePath tests

* Add source getter tests

* Add new data fixtures

* Add checkPath method

* Implement path validation logic

* Change validate return value to [] if valid

* Tests for basePath defaults and validation

* Add explanation for basePath

* Always throw error if basePath is invalid

* Correct comments

* Rename Resource.typeOfResourcePath to Resource.type

* Change validate return value to true

* Adjust tests to expect true from Profile.validate
@roll roll added duplicate and removed duplicate labels Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants