Better error validation for crx package manager tools

kitarek commented 10 years ago

Example:

$ mksh ./cqrev -u admin yyy ; echo $?
000<crx version="2.3.42" user="admin" workspace="crx.default">
<request>
    <param name="cmd" value="uninst"/>
    <param name="name" value="yyy"/>
  </request>
  <response>
    <data>
      <log>
Uninstalling content...
      </log>
    </data>
    <status code="500">com.day.jcr.vault.packaging.PackageException: Unable to uninstall package. No snapshot present.</status>
  </response>
</crx>
0

In that case exit code should be other than 0.

toniedzwiedz commented 5 years ago

This happens because the underlying curl , e.g.

curl -s -u "admin:admin" -H "Referer: http://localhost:4502/crx/packmgr" -F cmd=inst -F group=backups -F name=no-such-package.zip http://localhost:4502/crx/packmgr /service.jsp

yields an HTTP 200 response with the following response body:

<crx version="1.8.11" user="admin" workspace="crx.default">
  <request>
    <param name="cmd" value="inst"/>
    <param name="group" value="backups"/>
    <param name="name" value="no-such-package.zip"/>
  </request>
  <response>
    <data>
    </data>
    <status code="500">java.lang.IllegalArgumentException: Package 'backups:no-such-package.zip' does not exist.</status>
  </response>
</crx>

This looks like a bug in CRX as the "status code" in the body (500) is misaligned with the actual HTTP status code (200).

In my opinion, both these codes are wrong and AEM should respond with a code from the 4xx family. The request specifies a wrong package name and this can be addressed by the client. It's clearly not a success and it's not a server problem.

I'll see if I can raise a Day Care ticket.

In the meantime, the actual result can be obtained from the XML response. One way to o that would be to use xmllint (provided that it's installed)

For example:

curl -s -u "admin:admin" -H "Referer: http://localhost:4502/crx/packmgr" -F cmd=inst -F group=backups -F name=no-such-package.zip http://localhost:4502/crx/packmgr /service.jsp | xmllint --xpath "string(//status/@code)" -

will extract 500

toniedzwiedz commented 5 years ago

The current AEM documentation mentions a slightly different way to install a package.

curl -u admin:admin -F cmd=install http://localhost:4502/crx/packmgr/service/.json/etc/packages/my_packages/test.zip

where my_packages is the group and test is the package name. However, this API also returns a 200 response in case of failure.

curl -s -u "admin:FCiCPGPXb7GNooqe" -F cmd=install http://localhost:4502/crx/packmgr/service/.json/etc/packages/backups/not-existent.zip --write-out "%{http_code}"

{"success":false,"msg":"no package"}
200

It uses JSON instead of XML and does not wrongly mention HTTP 500 in the response body but it's still not an error response as far as HTTP is concerned.

pun-ky commented 5 years ago

Even if status codes are wrong, changing that could cause regression in clients connecting to aem which are already addressing that case.

jwadolowski commented 5 years ago

I don't agree with that. HTTP status codes exist for a reason and HTTP/1.1 200 OK in case of unsuccessful package installation is just not correct. It is a bug in Package Manager, however it's quite unlikely to be fixed anytime soon. If package you try to install does not exist (that was @toniedzwiedz's use case) 404 would be much more appropriate.

I do agree though that both response code and response body should be validated to deterimne whether given operation was or wasn't successful. I can imagine 200 with JSON body that says - "you know what, the package was installed, but a few minor issues was encountered along the way". It's up to the client to decide if such a warning is severe enough to fail the entire operation.

jwadolowski commented 5 years ago

Ran into similar issue today (exit code: 0):

$ cqrun -i http://localhost:4502 -u **** -p **** -g backups package-name
<crx version="1.8.11" user="****" workspace="crx.default">
...
  <response>
Installing content...
Creating snapshot for package group:package-name:20191024.1571889601
A META-INF
A META-INF/MANIFEST.MF
...
    <status code="500">java.io.IOException: No space left on device</status>
  </response>
</crx>

jwadolowski commented 5 years ago

Additionally this issue may get resolved once #47 is implemented.

wttech / CQ-Unix-Toolkit

Better error validation for crx package manager tools #24