#41 table relative paths #48

dumyan · 2017-01-16T16:48:04Z

Resolves #41 by adding basePath for remote datapackages and after that using url.resolve in Resources class to point to the relative resource.

dumyan · 2017-01-17T02:12:29Z

test/datapackage.js

+    })
+
+    it('URL resource', async () => {
+      const descriptor = 'https://dev.keitaro.info/dpkjs/datapackage.json'


This is temporary, we will change it to github once this branch is merged.

roll

Looks good 👍 I've added some comments.

roll · 2017-01-17T06:57:31Z

test/resource.js

+        path: resourcePath,
+      }, basePath)
+
+      assert(resource.source === path.normalize(`${basePath}/${resourcePath}`))


Could you change to resource.source === PATH_STRING - we need to be sure it returns exactly the same result we expect. If there is an error in "path.normalize(${basePath}/${resourcePath})" than we just replicate it in tests. But we need to test correctness of this code also.

roll · 2017-01-17T06:58:28Z

test/resource.js

+        path: resourcePath,
+      }, baseURL)
+
+      assert(resource.source === url.resolve(baseURL, resourcePath))


See above I suppose better will be write resource.source === EXPECTED_URL.

roll · 2017-01-17T07:00:17Z

src/resource.js

+        return url.resolve(this._basePath, source)
+      }
+
+      return path.normalize(`${this._basePath}/${source}`)


I suppose we better use something like path join instead of string interpolation to make it cross-platform.

roll · 2017-01-17T07:02:22Z

src/resource.js

@@ -71,12 +72,19 @@ export default class Resource {
   * @returns {String|Array|Object}
   */
  get source() {


This function logic seems a little bit complicated - may be it's possible to make it more straightforward using comments and groups? We also should analyze it for vulnerabilities (accessing e.g. /etc/passwd using relative paths) so this code should be pretty clear to understand.

For example:

// Inline data if (this.type === 'inline') return source; // Local data ... // Remote data ...

As for relative paths - it should be addresed on the spec level first. It already has a restriction of usage of dot-paths, but it is not enough (forbids usage only in the start of that path)

@Fak3
One of the spec WG meetings has decided to ban any traverses outside of datapackage.json basedir for local paths (no ^/ and ..). So we consider it as a rule just waiting official finalization.

@Fak3 @roll

both comments have been addressed. Path checking searches for ^/, and occurences of .. or .\. etc (backslash is being ignored when travesting dirs, so it's first taken out of the path).

roll · 2017-01-18T07:32:14Z

@dumyan
One other thing - I suppose we need much more tests for path resolutions. Could you write a few additional tests (for resource and datapackage if needed) with comprehensive list of combinations (basePath/path) to cover all edge cases? It could look like this for example:

it('resource source/type correctly works with base path', async () => {
  const paths = [
    {base: 'folder1', path: 'data/data.csv', source: 'folder1/data/data.csv', type: 'local'},
    // ... like 10 more)
  ]
  paths.forEach({base, path, source, type} => {
    const resource = new Resource({path}, base)
    assert(resource.source === source)
    assert(resource.type === type)
  })
})

it('resource source/type correctly works without base path', async () => {

The same technique could be used for other tests to ensure Resource.source/type works correctly on all possible input data.

roll

@dumyan
I've reviewed latest changes.

roll · 2017-01-24T08:27:55Z

src/datapackage.js

-      self._basePath = basePath || self._getBasePath(descriptor)
+
+      // Check if basePath is valid and throw error if needed
+      const basePathErrors = Utils.checkPath(basePath)


We have to always raise on bad base path. valid/errors/raiseInvalid is only for descriptor validation.

roll · 2017-01-24T08:31:57Z

src/profiles.js

@@ -73,7 +73,7 @@ export default class Profiles {

      const validation = tv4.validateMultiple(data, schema)
      if (validation.valid) {
-        return true


return True is correct. See validate - frictionlessdata/software-legacy#2

roll · 2017-01-24T09:00:09Z

src/resource.js

@@ -46,15 +45,14 @@ export default class Resource {
   *
   * @returns {String}
   */
-  get type() {
+  get typeOfResourcePath() {


Please follow frictionlessdata/software-legacy#2 (Resource.type for now)

roll · 2017-01-24T09:04:54Z

src/resource.js


-    return this.descriptor[this._sourceKey]
+    } else if (this._sourceKey === 'path' && this._basePath === '') {
+      // Local path, no basePath provided


Not sure why this.__sourceKey === 'path' means that a local path:

get _sourceKey() { const inlineData = this.descriptor.data if (inlineData) { return 'data' } return 'path' }

Based on this code remote path also satisfies this condition.

roll · 2017-01-24T09:07:03Z

src/resource.js

+
+      // we need to check the validity of the paths here because one can use
+      // only the Resource class to read file contents with `table`
+      if (this._validPaths) {


See basePath comment in constructor

@roll

Error is always thrown here if the paths are invalid.

roll

It seems everything work correct 👍

dumyan self-assigned this Jan 16, 2017

dumyan requested a review from roll January 16, 2017 16:48

dumyan added the review label Jan 16, 2017

dumyan commented Jan 17, 2017

View reviewed changes

roll requested changes Jan 17, 2017

View reviewed changes

roll mentioned this pull request Jan 18, 2017

Buggy resource path resolution #52

Closed

dumyan added 13 commits January 23, 2017 22:57

Handle relative resource paths for URLs

34f27c3

Add remote relative resource path test

70b957a

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

142f866

New datapackage for remote resource URL tests

75030a3

Make _getBasePath static

ac6e9e1

Add _getBasePath tests

0e111e1

Add source getter tests

ede87ef

Add new data fixtures

da6e15e

Add checkPath method

cbed4d2

Implement path validation logic

c1b1d8f

Change validate return value to [] if valid

78b2d51

Tests for basePath defaults and validation

1979108

Add explanation for basePath

d58bd14

roll requested changes Jan 24, 2017

View reviewed changes

dumyan added 3 commits January 24, 2017 12:51

Always throw error if basePath is invalid

e1b3773

Correct comments

97dbcb9

Rename Resource.typeOfResourcePath to Resource.type

611f2e8

dumyan force-pushed the #41-table-relative-paths branch from 82daa70 to 611f2e8 Compare January 24, 2017 12:41

dumyan added 2 commits January 24, 2017 14:07

Change validate return value to true

3558b62

Adjust tests to expect true from Profile.validate

07a9f1d

roll approved these changes Jan 24, 2017

View reviewed changes

roll merged commit 58fa700 into frictionlessdata:master Jan 24, 2017

roll removed the review label Jan 24, 2017

dumyan deleted the #41-table-relative-paths branch January 25, 2017 02:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#41 table relative paths #48

#41 table relative paths #48

dumyan commented Jan 16, 2017 •

edited by roll

Loading

dumyan Jan 17, 2017

roll left a comment

roll Jan 17, 2017

roll Jan 17, 2017

roll Jan 17, 2017

roll Jan 17, 2017

Fak3 Jan 17, 2017

roll Jan 18, 2017 •

edited

Loading

dumyan Jan 24, 2017

roll commented Jan 18, 2017 •

edited

Loading

roll left a comment

roll Jan 24, 2017

roll Jan 24, 2017

roll Jan 24, 2017

roll Jan 24, 2017

roll Jan 24, 2017

dumyan Jan 24, 2017

roll left a comment

#41 table relative paths #48

#41 table relative paths #48

Conversation

dumyan commented Jan 16, 2017 • edited by roll Loading

Choose a reason for hiding this comment

roll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roll Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roll commented Jan 18, 2017 • edited Loading

roll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roll left a comment

Choose a reason for hiding this comment

dumyan commented Jan 16, 2017 •

edited by roll

Loading

roll Jan 18, 2017 •

edited

Loading

roll commented Jan 18, 2017 •

edited

Loading