Buggy resource path resolution

rufuspollock commented 7 years ago

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 commented 7 years ago

Thanks, it will be addressed here - https://github.com/frictionlessdata/datapackage-js/pull/48 (cc @dumyan)

rufuspollock commented 7 years ago

@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

frictionlessdata / datapackage-js

Buggy resource path resolution #52