Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Fixed #661: wrap app_name query in xdmp:eval to provide appropriate context #768

Merged

Conversation

grtjn
Copy link
Contributor

@grtjn grtjn commented May 28, 2017

Fixes #661

@grtjn grtjn added this to the May 2017 milestone May 28, 2017
@grtjn
Copy link
Contributor Author

grtjn commented May 31, 2017

In case someone wonders, a simple way to run a quick test is to copy the execute_query_8 into app_specific.rb, and run the test code I provided in this comment of #661: #661 (comment)

@RobertSzkutak
Copy link
Contributor

RobertSzkutak commented Jun 9, 2017

Having two problems with this PR...

  1. On the latest version of ML8, the code throws this error:
    ERROR: 500 "Internal Server Error"
    ERROR: {"errorResponse":{"statusCode":500, "status":"Internal Server Error", "messageCode":"XDMP-UNDFUN", "message":"XDMP-UNDFUN: (err:XPST0017) Undefined function xdmp:server-coordinate-system()", "messageDetail":{"messageTitle":"Undefined function"}}}

Seems like to fix this its time to finally introduce execute_query_9 ...

  1. When I set the db_name param to something other than the app content db (the value used in your second test) on the latest ML8 it throws this error:
    ERROR: 500 "Internal Server Error"
    ERROR: {"errorResponse":{"statusCode":500, "status":"Internal Server Error", "messageCode":"XDMP-NODB", "message":"XDMP-NODB: xdmp:database-name(xs:unsignedLong("0")) -- No database with identifier 0", "messageDetail":{"messageTitle":"No database"}}}

Too tired to figure this one out I will try to debug it in the morning.

EDIT : I noticed that when running issue 2 on port 8000 it also fails with this error when you use anything other than the "Documents" database. Possibly a bug in the REST API on latest ML8 on Windows?

@RobertSzkutak RobertSzkutak self-requested a review June 9, 2017 04:31
@grtjn
Copy link
Contributor Author

grtjn commented Jun 9, 2017

Thanks, good finds. I should have tested my code against other than latest. I'll poke some more at this..

@grtjn
Copy link
Contributor Author

grtjn commented Jun 9, 2017

Issue 2 might just be bad test code. The bit of code I referred to above tries to get the database-name, but does not anticipate modules-db being set to (file system), which is the case for no params, since that reverts to the Manage app-server. Use this code instead:

  def test
    logger.info "Execute query with no params:"
    r = execute_query(
      %Q{
        xquery version "1.0-ml";

        declare function local:database-name($id) {
          if ($id eq 0) then
            "(file system)"
          else
            xdmp:database-name($id)
        };

        "content-db: " || local:database-name(xdmp:database()),
        "modules-db: " || local:database-name(xdmp:modules-database()),
        "schemas-db: " || local:database-name(xdmp:schema-database()),
        "trigger-db: " || local:database-name(xdmp:triggers-database())
      }
    )
    r.body = parse_body r.body
    logger.info r.body
    logger.info ""

    logger.info "Execute query with db_name param:"
    r = execute_query(
      %Q{
        xquery version "1.0-ml";

        declare function local:database-name($id) {
          if ($id eq 0) then
            "(file system)"
          else
            xdmp:database-name($id)
        };

        "content-db: " || local:database-name(xdmp:database()),
        "modules-db: " || local:database-name(xdmp:modules-database()),
        "schemas-db: " || local:database-name(xdmp:schema-database()),
        "trigger-db: " || local:database-name(xdmp:triggers-database())
      },
      { :db_name => @properties['ml.content-db'] }
    )
    r.body = parse_body r.body
    logger.info r.body
    logger.info ""

    logger.info "Execute query with app_name param:"
    r = execute_query(
      %Q{
        xquery version "1.0-ml";

        declare function local:database-name($id) {
          if ($id eq 0) then
            "(file system)"
          else
            xdmp:database-name($id)
        };

        "content-db: " || local:database-name(xdmp:database()),
        "modules-db: " || local:database-name(xdmp:modules-database()),
        "schemas-db: " || local:database-name(xdmp:schema-database()),
        "trigger-db: " || local:database-name(xdmp:triggers-database())
      },
      { :app_name => @properties['ml.app-name'] }
    )
    r.body = parse_body r.body
    logger.info r.body
    logger.info ""
  end

Also make sure you bootstrap before you run that test, otherwise mentioned values will be wrong (details for Manage or some random app-server).

I'm investigating running exec_8 against ML8..

@grtjn grtjn force-pushed the 661-app_name-query-over-rest branch from 5ac49a4 to f156aaa Compare June 9, 2017 07:03
@grtjn
Copy link
Contributor Author

grtjn commented Jun 9, 2017

This should work better..

I'm printing warnings for ml7- if a referenced db_name or app_name is not found. The execute_query_5/7 seemed to be very forgiving, a bit too much I think.

The execute_query_8 however is not forgiving, no fallback if db_name or app_name refers to non-existing items. I added a check upfront, and raise an exception if non-existing. The effect is similar if we don't do this, but the feedback for the user is cleaner this way.

I also applied a trick with fn:function-lookup to be server-version agnostic. All seems to run smooth against ml7, ml8, and ml9 now..

@grtjn
Copy link
Contributor Author

grtjn commented Jun 9, 2017

@RobertSzkutak Can you give it another spin?

@RobertSzkutak
Copy link
Contributor

Everything looks good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants