Review of code

[pwalsh]

• can we remove the zappa stuff completely and host on docker cloud? ( App randomly throws error messages #258 )
  • Will fix - see Move off AWS Lambda / API Gateway #301 [FIXED]
• As noted in the UI review, the CLI does not work if one follows the instructions.
  • Agreed and working on fix atm - see Update dashboard instructions to use dpm rather than dpmpy #293 [FIXED]
• Where is the production config? https://github.com/frictionlessdata/dpr-api/blob/master/app/config.py
  • It would be identical to StageConfig, but we need one - agreed. See Create production configuration in flask #311 [FIXED]
• Not good to have a hard coded, publicly accessible secret key: https://github.com/frictionlessdata/dpr-api/blob/master/app/__init__.py#L62
  • Will fix - see API_KEY should be configurable in .env file #206 [FIXED]
• except Exception as e raise Exception is weird: https://github.com/frictionlessdata/dpr-api/blob/master/app/__init__.py#L71
  • Weird, but how else Log the desired message? Again there is reason this is done in this way. Probably WON'T FIX? [WONTFIX]
• Can we please do more specific exception handling: https://github.com/frictionlessdata/dpr-api/blob/master/app/__init__.py#L124
  • see Better exception handling #312 [WILL FIX]
• This logic is obviously broken (user always gets index, apart from first page after login flow): https://github.com/frictionlessdata/dpr-api/blob/master/app/site/controllers.py#L22
  • Working right now. Please take a look at Server does not know if Browser user is logged in or not #292 (comment) [FIXED]
• As discussed - there is a significant problem with the modeling of tags/versions. Will not go into it here as we discussed, but it breaks user experience at several levels: CLI, API, and UI
  • This is FIXED with Datapackage delete and purge commands are failing #295 [FIXED]
• There is an undesirable pattern through all this code of having general logic inlined with route handlers. As the codebase grows, this gets unmanageable very quickly, and leads to duplication of logic and introduction of bugs. example, have a README util that gets called in the handler, rather than inlining the README processing logic: https://github.com/frictionlessdata/dpr-api/blob/master/app/site/controllers.py#L100
  • AGREED but Right now, I don’t think is a priority. [WILLFIX]see Remove general logic that is inlined with route handlers #328
• I'm still no clearer on why this is called "catalog" with the new docstring: https://github.com/frictionlessdata/dpr-api/blob/master/app/site/models.py#L11
  • AGREED: delete this file. Not needed anymore. [WILLFIX] Remove class Catalog from code base #326
• Unfortunately the search is really bad: https://github.com/frictionlessdata/dpr-api/blob/master/app/search/models.py#L23 but we can get much better without introducing a dedicated search engine: http://rachbelaid.com/postgres-full-text-search-is-good-enough/
  • AGREED We know this and we know there is plenty to improve. Will go and do: search on title and readme with very heavy weighting on title. [WILLFIX] Search does proper FTS over title and maybe README #327
• Still not fixed: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L31
  • Probably remove this code block, as we are doing data validate at client side. See Validate resources data after upload. #190. [WONTFIX]
• Do we really want to read any object from S3, rather than iterate over a stream? https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L68
  • This is for datapackage.json and readme. These are very small data. Probably does not really need streaming here. [WONTFIX]
• This really just duplicates logic from the above method, with no clear reason why (could have a more general method to get an object from S3): https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L72
  • This is completely different. This one scans the directory for README prefix [WONTFIX]
• We know there is a problem with deletion, so please investigate deeply: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L155
  • Fixed with Datapackage delete and purge commands are failing #295 [FIXED]
• This seems like a bad place to silently swallow an exception: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L177
  • see Better exception handling #312 [WILL FIX]
• Likewise swallowing any exception here: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L204
  • see Better exception handling #312 [WILL FIX]
• On the above two points, and an earlier one - If we handle Exception and return False, especially on operations such as reading, writing and deleting user data, we have a real problem waiting to appear in unexpected ways. Please review the whole codebase for this issue. Another example: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L228
  • see Better exception handling #312 [WILL FIX]
• Is it not better (performance, indexing) to use posgres' native enum? https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L255
  • We do not want to use Pgsql Json type as. So probably WOULD not FIX [WONTFIX]
• What is going on here? meaning, we have a variable called "instance" that can return multiple records. This is concerning from a data integrity perspective (we should be able to expect a single match, and have the exception if we have multiple), and a naming perspective: https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L385
  • This was the case when (publisher, package, version) was unique key for Package model. So this method was returning multiple packages. Can change this now. As we created (publisher, package) unique key for Package model. So combination of publisher and package will return one row now. See Use sql-alchemy's one method in place of first #313 [FIXED]
• Is staticmethod the best implementation for many of the methods on here? https://github.com/frictionlessdata/dpr-api/blob/master/app/package/models.py#L239
  • In my opinion it is good. These are logical grouping of utility functions associated with the model. We could make them @classmethod. But we do not get much benefit. [WONTFIX]
• As github does not guarantee we will get an email, will this not break in certain conditions? https://github.com/frictionlessdata/dpr-api/blob/master/app/auth/controllers.py#L118
  • This will break if username and email both are none. This will not break, as github always provide username [WONTFIX]
• user_name / username? https://github.com/frictionlessdata/dpr-api/blob/master/app/auth/controllers.py#L113
  • Github provide user name as key username. Not a problem [WONTFIX]~~
• Why can't we get one on email? Having a filter here and picking first concerns me in terms of data integrity (do we expect there could be multiple records with the same email?): https://github.com/frictionlessdata/dpr-api/blob/master/app/auth/controllers.py#L137
  • It was introduced when we had Auth0. Where email was not unique. We were relying on auth0 id. It is not needed now. Will have to create issue to remove that. See Remove ambiguous db call for user email #310 [FIXED]

[rufuspollock]

@pwalsh thank-you for this thorough and really useful review. We'll go through and look at each and address as appropriate.

[subhankarb]

@rufuspollock I updated the review with my investigation. Please take a look.

[rufuspollock]

Nearly done but a few outstanding items.

@zelima please can you link subissues into this so we get accurate totals.

[rufuspollock]

@zelima I think we can mark as FIXED once logic stuff is done today / monday. Just search will be left and I think we can close with comment that search is left see issue #327

[zelima]

FIXED. each issue is fixed or marked as wontfix. The only outstanding issue about search is #327