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

Add registry validator test; make 404 responses compatible with npm API #57

Closed

Conversation

bajtos
Copy link
Contributor

@bajtos bajtos commented Mar 17, 2014

The previous pull request #56 made the 404 response for unknown
package name correct, but broke the expected format of the 404 response
for unknown package version.

This pull request fixes the problem.

The first commit adds a test suite executing npm commands agains the Sinopia registry.

The second commit implements the necessary changes in Sinopia.

Miroslav Bajtoš added 2 commits March 14, 2014 11:51
Include Mocha tests from the validation suite in `npm test`.
The previous commit dabf5e1 made the 404 response for unknown
package *name* correct, but broke the expected format of the 404 response
for unknown package *version*.

This commit fixes the problem by sending 'not_found' response only
when the package was not found at all.

The following commits are partially reverted: df49fb8 dabf5e1
@rlidwka
Copy link
Owner

rlidwka commented Mar 20, 2014

How about reverting that entire patch?

Sinopia returns a nice error message that depend on an exact error that occured. In the future it might contain information about whether npm registry is reachable or not, or useful other info. Instead of that npm is returning its own error message losing this information.

Also, we're using sinopia behind nginx redirecting https://dev.local/sinopia/ to http://localhost:4873/, and I just found another issue with this:

[alex@dev tmp]$ npm install qwertyueioasvghyujww
npm http GET https://dev.local/sinopia/qwertyueioasvghyujww
npm http 404 https://dev.local/sinopia/qwertyueioasvghyujww
npm ERR! 404 'sinopia' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, or http url, or git url.

npm ERR! System Linux 2.6.32-431.el6.x86_64
npm ERR! command "node" "/usr/bin/npm" "install" "qwertyueioasvghyujww"
npm ERR! cwd /tmp
npm ERR! node -v v0.10.26
npm ERR! npm -v 1.3.6
npm ERR! code E404
npm ERR! 
npm ERR! Additional logging details can be found in:
npm ERR!     /tmp/npm-debug.log
npm ERR! not ok code 0

@bajtos
Copy link
Contributor Author

bajtos commented Mar 21, 2014

Sinopia returns a nice error message that depend on an exact error that occured. In the future it might contain information about whether npm registry is reachable or not, or useful other info. Instead of that npm is returning its own error message losing this information.

What I don't like about this approach is the stack trace printed by npm, which makes the error look scarier than it is. Users should not get stack traces in common situations like when they a mistype package name.

I suppose the right way how to address custom error messages is to put them in reason or perhaps details field of the json response and alter the npm client to include this field in error messages.

Also, we're using sinopia behind nginx redirecting https://dev.local/sinopia/ to http://localhost:4873/, and I just found another issue with this:

npm http 404 https://dev.local/sinopia/qwertyueioasvghyujww
npm ERR! 404 'sinopia' is not in the npm registry.

That's annoying :( However, it's a bug of the npm client and should be fixed there. I'll try to look into this problem next week and submit a pull request with a fix to the npm project.

How about reverting that entire patch?

I hope my explanation above convinced you that reverting the entire patch is not the best solution.

@rlidwka
Copy link
Owner

rlidwka commented Mar 22, 2014

Users should not get stack traces in common situations like when they a mistype package name.

If you mistype version or tag, npm will output stack trace. In fact, npm is outputting stack trace almost on any error. Why this one should be any different?

I suppose the right way how to address custom error messages is to put them in reason

npm ignores "reason" and doesn't write it to the user.

If error message have some vital information, it's better to display it to the user even though it's accompanied by a stack trace.

@rlidwka
Copy link
Owner

rlidwka commented Mar 22, 2014

I'll just ask this. Which error message is more useful?

$ npm install express
npm http GET https://example.org/express
npm http 404 https://example.org/express
npm ERR! 404 'express' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, or http url, or git url.
$ npm install express
npm http GET https://example.org/express
npm http 404 https://example.org/express
npm ERR! Error: no such package available: express (npm registry is down)
npm ERR!     at RegClient.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/npm-registry-client/lib/request.js:272:14)
npm ERR!     at Request.self.callback (/usr/local/lib/node_modules/npm/node_modules/request/request.js:129:22)
npm ERR!     at Request.EventEmitter.emit (events.js:98:17)
npm ERR!     at Request.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/request/request.js:873:14)
npm ERR!     at Request.EventEmitter.emit (events.js:117:20)
npm ERR!     at IncomingMessage.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/request/request.js:824:12)
npm ERR!     at IncomingMessage.EventEmitter.emit (events.js:117:20)
npm ERR!     at _stream_readable.js:920:16
npm ERR!     at process._tickCallback (node.js:415:13)

@bajtos
Copy link
Contributor Author

bajtos commented Mar 24, 2014

Users should not get stack traces in common situations like when they a mistype package name.

If you mistype version or tag, npm will output stack trace. In fact, npm is outputting stack trace almost on any error. Why this one should be any different?

$ npm -v
1.4.6
$ npm --registry=https://registry.npmjs.org/ install not-found
npm http GET https://registry.npmjs.org/not-found
npm http 404 https://registry.npmjs.org/not-found
npm ERR! 404 'not-found' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, or http url, or git url.

[cut: os, node, npm version]
$ npm --registry=https://registry.npmjs.org/ install debug@20.0.0
npm http GET https://registry.npmjs.org/debug/20.0.0
npm http 404 https://registry.npmjs.org/debug/20.0.0
npm ERR! Error: version not found: 20.0.0 : debug/20.0.0
npm ERR!     at RegClient.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/npm-registry-client/lib/request.js:254:14)
npm ERR!     at Request._callback (/usr/local/lib/node_modules/npm/node_modules/npm-registry-client/lib/request.js:192:65)
npm ERR!     at Request.self.callback (/usr/local/lib/node_modules/npm/node_modules/request/request.js:123:22)
npm ERR!     at Request.EventEmitter.emit (events.js:98:17)
npm ERR!     at Request.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/request/request.js:893:14)
npm ERR!     at Request.EventEmitter.emit (events.js:117:20)
npm ERR!     at IncomingMessage.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/request/request.js:844:12)
npm ERR!     at IncomingMessage.EventEmitter.emit (events.js:117:20)
npm ERR!     at _stream_readable.js:920:16
npm ERR!     at process._tickCallback (node.js:415:13)
npm ERR! If you need help, you may report this *entire* log,
npm ERR! including the npm and node versions, at:
npm ERR!     <http://github.com/npm/npm/issues>

[cut: os, node, npm version]

I wanted to write that you are not right, but alas, it's really printing stack traces :(

In that case you are probably right, let's revert #56 and close this one. I'll try to figure out how to fix my test suite to make the error message check less strict.

@bajtos bajtos closed this Mar 24, 2014
@bajtos
Copy link
Contributor Author

bajtos commented Mar 24, 2014

BTW, when the package is not found because the public registry is down, you should probably return 504 Gateway Timeout instead of 404.

The server, while acting as a gateway or proxy, did not receive a timely response from the upstream server specified by the URI (e.g. HTTP, FTP, LDAP) or some other auxiliary server (e.g. DNS) it needed to access in attempting to complete the request.

rlidwka added a commit that referenced this pull request Mar 29, 2014
STRML pushed a commit to STRML/sinopia that referenced this pull request Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants