apigee / apigeelint

Static code analysis for Apigee proxy bundles to encourage API developers to use best practices and avoid anti-patterns.
Apache License 2.0
92 stars 71 forks source link

Differences in filepath format in output across platforms #353

Closed andythehood closed 1 year ago

andythehood commented 1 year ago

The filepath format in the results output is not consistent across MacOS and Windows. On Windows, the file path is the fully qualified file system path, i.e. starting with C:. Whereas on Mac, it is neither a fully qualified path starting from root (/) nor is it a relative path i.e. starting with ./apiproxy. Instead it shows as an absolute path, but starts from the root of the bundle, i.e. /apiproxy/

On MacOS (also same on Cloudshell):

andyhood:~/Projects/Apigee/api-platform-samples/default-proxies/helloworld> apigeelint -s apiproxy -f stylish.js

/apiproxy/targets/default.xml 0:0 warning TargetEndpoint (default) is using URL (http://mocktarget.apigee.net), using a Target Server simplifies CI/CD TD002

On Windows:

PS C:\Users\AndyHood\Projects\Apigee\api-platform-samples\default-proxies\helloworld> apigeelint -s apiproxy -f stylish.js

C:\Users\AndyHood\Projects\Apigee\api-platform-samples\default-proxies\helloworld\apiproxy/targets/default.xml 0:0 warning TargetEndpoint (default) is using URL (http://mocktarget.apigee.net), using a Target Server simplifies CI/CD TD002

DinoChiesa commented 1 year ago

What would be a good solution here? Do you suggest to always use the relative path? Eg

apiproxy/targets/default.xml

?

ssvaidyanathan commented 1 year ago

I would say - let be consistent with whatever we choose

andythehood commented 1 year ago

My preference would be to use the absolute path - because it makes hyperlinking back to the source file easier in that VScode extension I wrote the other day (shameless plug).

But relative, i.e. /apiproxy/targets/default.xml would work just as well.

I found the cause of the inconsistency across platform is due to code like this: Policy.js:34 filePath: this.filePath.substring(this.filePath.indexOf('/apiproxy'))

with the issue being that on Windows, the path would be \apiproxy not /apiproxy, and so the substring doesn't match. And there's a few other places in the code that hardcode '/' as the path.

To fix this, you could modify the code to use path.sep everywhere, but the best thing I can suggest is to standardise on forward slash for filepaths, even on Windows (as Windows is fine with that), otherwise you get paths like \default-proxies\helloworld\apiproxy/targets/default.xml with a mix of forward and back slashes.

The latter can be achieved with this code: Bundle.js:88 bundle.root = path.resolve(config.source.path).split(path.sep).join('/');

If you do this, and choose to use relative paths, I think that's the only change that would need to be made.

[Although filePath.indexOf('/apiproxy') should really be using lastIndexOf() instead of indexOf]

DinoChiesa commented 1 year ago

Work in progress. This shows relative paths. I could convert it to full paths. WDYT?

PS C:\Users\dchiesa\apigeelint>  .\node_modules\.bin\apigeelint -s C:\Users\dchiesa\apiproxy-example\apiproxy -f stylish.js

apiproxy/targets/target-1.xml
  13:3  warning  Missing TrustStore in SSLInfo                                                                                 TD004
   0:0  warning  TargetEndpoint (target-1) is using URL (https://echo.dinochiesa.net), using a Target Server simplifies CI/CD  TD002

apiproxy/policies/CORS-1.xml
  0:0  error  The policy type (CORS) is not available in the profile apigee  PO028

✖ 3 problems (1 error, 2 warnings)

Here is the full-paths version:

PS C:\Users\dchiesa\apigeelint>  .\node_modules\.bin\apigeelint -s C:\Users\dchiesa\apiproxy-example\apiproxy -f stylish.js

C:/Users/dchiesa/apiproxy-example/apiproxy/targets/target-1.xml
  13:3  warning  Missing TrustStore in SSLInfo                                                                                 TD004
   0:0  warning  TargetEndpoint (target-1) is using URL (https://echo.dinochiesa.net), using a Target Server simplifies CI/CD  TD002

C:/Users/dchiesa/apiproxy-example/apiproxy/policies/CORS-1.xml
  0:0  error  The policy type (CORS) is not available in the profile apigee  PO028

✖ 3 problems (1 error, 2 warnings)

You can see in that example I took your advice and converted everything to forward slashes. That is also not necessary; we could convert them to the opposite.

What do other multi-platform command line tools do?

ssvaidyanathan commented 1 year ago

decided to use full path

andythehood commented 1 year ago

That's great, thanks.

ssvaidyanathan commented 1 year ago

I will close once we push the release

ssvaidyanathan commented 1 year ago

Fixed and released in v2.34.0