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

When the connection to database fails, the API should return a valid response #7903

Closed
mattab opened this Issue May 13, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@mattab
Member

mattab commented May 13, 2015

When Piwik cannot connect to the database, the API should return a valid response.

Reproduce:

  • Edit config.ini.php and set a wrong username for the database eg. xyytueyeat
  • Open index.php?module=API&method=API.getPiwikVersion&format=xml
  • Got: HTML error output
  • Expected:
    • HTTP error code 500,
    • and the API call returns a valid eg. XML response that returns the API error reponse eg.
<?xml version="1.0" encoding="utf-8" ?>
<result>
    <error message="Cannot connect to the database: SQLSTATE[28000] [1045] Access denied for user 'xyytueyeat'@'localhost' (using password: YES)" />
</result>

Steps:

  • Add test case
  • Fix issue

Refs #7901 #7902

@saleemkce

This comment has been minimized.

Show comment
Hide comment
@saleemkce

saleemkce May 23, 2015

Contributor

@mattab About "Add test case". I have issues with running test cases from CLI http://forum.piwik.org/read.php?15,126943. There is no problem when web is concerned. Could you show me some sample test case files which closely resembles FrontControllerTest or similar test case file so that I could write test cases properly for this issue?

Contributor

saleemkce commented May 23, 2015

@mattab About "Add test case". I have issues with running test cases from CLI http://forum.piwik.org/read.php?15,126943. There is no problem when web is concerned. Could you show me some sample test case files which closely resembles FrontControllerTest or similar test case file so that I could write test cases properly for this issue?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab May 24, 2015

Member

Hi @saleemkce

I have issues with running test cases from CLI http://forum.piwik.org/read.php?15,126943

I think the issue is due to the fact you are on Windows. Unfortunately none of the core developers use Windows (we use Ubunty or Mac OS). Feel free to create a separate issue to report that running tests don't work on Windows. Maybe you even have an idea what causes this problem? (eg. maybe it's a path problem as we build paths using forward slash)

Member

mattab commented May 24, 2015

Hi @saleemkce

I have issues with running test cases from CLI http://forum.piwik.org/read.php?15,126943

I think the issue is due to the fact you are on Windows. Unfortunately none of the core developers use Windows (we use Ubunty or Mac OS). Feel free to create a separate issue to report that running tests don't work on Windows. Maybe you even have an idea what causes this problem? (eg. maybe it's a path problem as we build paths using forward slash)

@mattab mattab added this to the 2.15.0 milestone Jul 15, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Jul 29, 2015

Member

I'm not sure if I understand. Why should we return a valid response in this case? If it's to avoid exposing a message like Access denied for user 'xyytueyeat'@'localhost' (using password: YES) we should maybe rather catch this exception and use a different message?

Member

tsteur commented Jul 29, 2015

I'm not sure if I understand. Why should we return a valid response in this case? If it's to avoid exposing a message like Access denied for user 'xyytueyeat'@'localhost' (using password: YES) we should maybe rather catch this exception and use a different message?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 9, 2015

Member

I'm not sure if I understand. Why should we return a valid response in this case?

The idea is to keep API returning "valid" data at all times, even when Piwik DB is down, so the API is more consistent even in edge cases.

we should maybe rather catch this exception and use a different message?

If you think that's better than showing the message, sounds good. Maybe we could also wrap the actual error message within another generic one eg. Piwik could not connect to your analytics database. Error was %s

Member

mattab commented Aug 9, 2015

I'm not sure if I understand. Why should we return a valid response in this case?

The idea is to keep API returning "valid" data at all times, even when Piwik DB is down, so the API is more consistent even in edge cases.

we should maybe rather catch this exception and use a different message?

If you think that's better than showing the message, sounds good. Maybe we could also wrap the actual error message within another generic one eg. Piwik could not connect to your analytics database. Error was %s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment