[Corona] JSON validation

Review Request #392 — Created Nov. 9, 2015 and updated

smillernl
Lunr
feature/json_validation
b98522b...
lunr
Corona: JSON validation


  • 1
  • 0
  • 8
  • 0
  • 9
Description From Last Updated
This extra locator parameter looks out of place :/ Just a thought: Would it be an option to have this ... tardypad tardypad
smillernl
tardypad
  1. 
      
  2. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    First I don't think the verification should be done only on a 200 return code
    Some webservices might return useful data using a different code
    The question is whether we should do it for all status codes or maybe just 2xx ones.

    Second, deciding to do the verification only if the schema file exists, is not flexible enough.
    We will have schema files defined there to be used by the clients only as a source of information
    Those should not be used for verification (processing too long or other reason)
    And we don't want to duplicate those schema files (one place for verification, one for documentation)
    I would go for configuration usage here

    1. what other 2xx codes exist in our code?

    2. potentially all

      By the way coming back to this,
      for some status code the web server doesn't send body data
      so the verification can definitely NOT be done for them

  3. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    I think you can remove all the comments here

  4. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     
     

    snake_case the variables

  5. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    Why not using $json directly there?

  6. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    you should not override the message here
    there is most likely more useful information in there already

  7. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    This is not related to JSON validation, that needs to be in a different review

    Already few comments about it though:
    - I would change the variable name from $time to $initial_time or such (to be more explicit)
    - why only for web requests and not cli?

    1. This was initially for benchmarking. I don't even know if it'd be useful.

    2. ah this is just the time of the print page / verification
      Well indeed it is not useful, remove it

  8. src/Lunr/Corona/JsonView.php (Diff revision 1)
     
     

    you already have done this before line 83

  9. support/setup.sh (Diff revision 1)
     
     

    there is a new version now 1.6.1 :)

  10. 
      
smillernl
smillernl
Review request changed

Commit:

-a57250e5dd469336348fe3cf6447e9dd726dcb4d
+b98522b17f13444d10b33a0445e77703e7ce6711

Diff:

Revision 3 (+84 -4)

Show changes

tardypad
  1. 
      
  2. src/Lunr/Corona/JsonView.php (Diff revision 3)
     
     

    This extra locator parameter looks out of place :/

    Just a thought:
    Would it be an option to have this validation feature as part of a child class like JsonValidationView?

    So that we don't have this extra parameter and we don't break the class either
    by replacing all constructor parameter with only the locator

    That would then leave each individual project to implement the way
    they want to choose which view to use (whether via config, json schema file existence, ...).

    The only thing needed in that JsonView class would be like a hook extra_result_process(&$json)
    called before the output, that would do nothing by default
    (come with a better name than this :)

    1. How about a completely different approach? Let me outline:

      How about a SchemaVerification class that takes the View as parameter, either in the Constructor or in a verify() method. The verify() method would then "somehow" fetch the information from the view. There's multiple ways to get to the data, and I think we should offer multiple options, like fetching from the output buffer, or having a get_raw_data() method in the View class.

  3. 
      
Loading...