Bug 17008 - REST API: Correct data types in Swagger
Summary: REST API: Correct data types in Swagger
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords:
Depends on: 17032
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-01 07:19 UTC by Lari Taskula
Modified: 2017-12-07 22:16 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17008: Test to compare Swagger property types against column data types (5.28 KB, patch)
2016-08-15 16:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (5.92 KB, patch)
2016-08-16 10:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects (9.24 KB, patch)
2016-08-16 13:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (4.41 KB, patch)
2016-08-16 13:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Fix current data types (7.54 KB, patch)
2016-08-16 13:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects (10.15 KB, patch)
2016-08-18 11:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Fix current data types (7.55 KB, patch)
2016-08-18 11:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (4.41 KB, patch)
2016-08-18 11:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects (10.15 KB, patch)
2016-10-12 10:54 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Fix current data types (7.82 KB, patch)
2016-10-12 10:55 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (4.90 KB, patch)
2016-10-12 10:56 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Fix current data types (8.45 KB, patch)
2016-10-12 11:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (4.90 KB, patch)
2016-10-12 11:08 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects (10.15 KB, patch)
2016-10-12 13:49 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Fix current data types (8.33 KB, patch)
2016-10-12 13:49 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17008: Test to compare Swagger property types against column data types (4.90 KB, patch)
2016-10-12 13:49 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects (10.23 KB, patch)
2016-11-08 21:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17008: Fix current data types (8.39 KB, patch)
2016-11-08 21:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17008: Test to compare Swagger property types against column data types (4.96 KB, patch)
2016-11-08 21:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17008: followup - fix cityid type (729 bytes, patch)
2016-11-08 21:32 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-08-01 07:19:39 UTC
Currently some data types are oddly defined in Swagger. For example borrowernumber is defined as a string, even though it and many other *numbers should be integers/numbers.
Comment 1 Lari Taskula 2016-08-15 16:16:33 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2016-08-16 10:30:11 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2016-08-16 13:42:21 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2016-08-16 13:45:00 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2016-08-16 13:45:34 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2016-08-18 11:28:06 UTC
Created attachment 54561 [details] [review]
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects

This patch adds new subroutine "swaggerize" for Koha::Object and Koha::Objects.

By matching column data types with corresponding Swagger data type, we can e.g.
numify values in order to be able to return borrowernumber:123 instead of
borrowernumber:"123".

Before this patch, we usually returned an unblessed representation of the
Koha-object in controller by calling e.g.
  $c->$cb($patron->unblessed, 200);

This would cause the returned JSON to have { "borrowernumber": "123" ... }, or
in other words everything as strings (apart from undefs, which would be null).

Now, we also return an unblessed representation of the Koha-object, but where
numbers are actually numified simply by calling e.g.
  $c->$cb($patron->swaggerize, 200);

Here our returned JSON will have { "borrowernumber": 123 ... } in "integer" data
type of Swagger, as well as every other column that is defined to be an integer.
Decimals, doubles and floats should have "number" data type and will also be
numified for the returned JSON.

To test:
1. Apply this patch and run t/db_dependent/Koha/Util/Swagger.t
2. Apply REST of the patches and follow their test plan for seeing how this
   works in practice
Comment 7 Lari Taskula 2016-08-18 11:28:25 UTC
Created attachment 54562 [details] [review]
Bug 17008: Fix current data types

This patch changes current Swagger definitions for patrons and holds to have
data types corresponding to column data types in their database tables.

To test:
1. GET http://yourlibrary/api/v1/patrons/YYY where YYY is existing borrowernumber
2. Observe that numbers / integers are in string data type.
3. Apply this patch.
4. Repeat step 1.
5. Observe that numbers / integers are now actually numbers / integers.
Comment 8 Lari Taskula 2016-08-18 11:28:37 UTC
Created attachment 54563 [details] [review]
Bug 17008: Test to compare Swagger property types against column data types

This patch adds a test that iterates through each definition-object in Swagger
and compares their properties' data types to corresponding Koha-object's column's
data type by using Koha::Util::Swagger::column_type_to_swagger_type().

This test will also check if the column is nullable and will complain
unless property has also "null" defined in its type.

To test:
1. Run t/db_dependent/api/v1/swagger/definitions.t
2. Observe that tests pass (if not, open a Bug to fix issues)
3. Modify api/v1/definitions/patron.json cardnumber to be type "integer".
4. Run the test again.
5. Observe that test fails and will complain about incorrect data type.
Comment 9 Jonathan Druart 2016-10-12 09:34:23 UTC
CONFLICT (modify/delete): api/v1/definitions/patron.json deleted in 096078a5084dfb5d5fad9ce4e62ecef94140958a and modified in Bug 17008: Fix current data types. Version Bug 17008: Fix current data types of api/v1/definitions/patron.json left in tree.
CONFLICT (modify/delete): api/v1/definitions/hold.json deleted in 096078a5084dfb5d5fad9ce4e62ecef94140958a and modified in Bug 17008: Fix current data types. Version Bug 17008: Fix current data types of api/v1/definitions/hold.json left in tree.
Comment 10 Lari Taskula 2016-10-12 10:54:47 UTC
Created attachment 56247 [details] [review]
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects
Comment 11 Lari Taskula 2016-10-12 10:55:10 UTC
Created attachment 56248 [details] [review]
Bug 17008: Fix current data types

This patch changes current Swagger definitions for patrons and holds to have
data types corresponding to column data types in their database tables.

To test:
1. GET http://yourlibrary/api/v1/patrons/YYY where YYY is existing borrowernumber
2. Observe that numbers / integers are in string data type.
3. Apply this patch and minify swagger.json
perl misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json -d api/v1/swagger/swagger.min.json
4. Repeat step 1.
5. Observe that numbers / integers are now actually numbers / integers.
Comment 12 Lari Taskula 2016-10-12 10:56:55 UTC
Created attachment 56249 [details] [review]
Bug 17008: Test to compare Swagger property types against column data types
Comment 13 Lari Taskula 2016-10-12 10:57:27 UTC
Rebased on top of master.
Comment 14 Lari Taskula 2016-10-12 11:08:23 UTC
Created attachment 56250 [details] [review]
Bug 17008: Fix current data types

Fixed nullability for "branchcode".
Comment 15 Lari Taskula 2016-10-12 11:08:40 UTC
Created attachment 56251 [details] [review]
Bug 17008: Test to compare Swagger property types against column data types
Comment 16 Lari Taskula 2016-10-12 13:49:03 UTC
Created attachment 56279 [details] [review]
Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects

This patch adds new subroutine "swaggerize" for Koha::Object and Koha::Objects.

By matching column data types with corresponding Swagger data type, we can e.g.
numify values in order to be able to return borrowernumber:123 instead of
borrowernumber:"123".

Before this patch, we usually returned an unblessed representation of the
Koha-object in controller by calling e.g.
  $c->$cb($patron->unblessed, 200);

This would cause the returned JSON to have { "borrowernumber": "123" ... }, or
in other words everything as strings (apart from undefs, which would be null).

Now, we also return an unblessed representation of the Koha-object, but where
numbers are actually numified simply by calling e.g.
  $c->$cb($patron->swaggerize, 200);

Here our returned JSON will have { "borrowernumber": 123 ... } in "integer" data
type of Swagger, as well as every other column that is defined to be an integer.
Decimals, doubles and floats should have "number" data type and will also be
numified for the returned JSON.

To test:
1. Apply this patch and run t/db_dependent/Koha/Util/Swagger.t
2. Apply REST of the patches and follow their test plan for seeing how this
   works in practice
Comment 17 Lari Taskula 2016-10-12 13:49:28 UTC
Created attachment 56280 [details] [review]
Bug 17008: Fix current data types

This patch changes current Swagger definitions for patrons and holds to have
data types corresponding to column data types in their database tables.

To test:
1. GET http://yourlibrary/api/v1/patrons/YYY where YYY is existing borrowernumber
2. Observe that numbers / integers are in string data type.
3. Apply this patch
4. Repeat step 1.
5. Observe that numbers / integers are now actually numbers / integers.
Comment 18 Lari Taskula 2016-10-12 13:49:43 UTC
Created attachment 56281 [details] [review]
Bug 17008: Test to compare Swagger property types against column data types

This patch adds a test that iterates through each definition-object in Swagger
and compares their properties' data types to corresponding Koha-object's column's
data type by using Koha::Util::Swagger::column_type_to_swagger_type().

This test will also check if the column is nullable and will complain
unless property has also "null" defined in its type.

To test:
1. Run t/db_dependent/api/v1/swagger/definitions.t
2. Observe that tests pass (if not, open a Bug to fix issues)
3. Modify api/v1/swagger/definitions/patron.json cardnumber to be type "integer".
4. Run the test again.
5. Observe that test fails and will complain about incorrect data type.
Comment 19 Lari Taskula 2016-10-12 13:50:48 UTC
Removed minifying step from test plans after Bug 17432.
Comment 20 Josef Moravec 2016-11-08 21:32:27 UTC
Created attachment 57352 [details] [review]
[SIGNED-OFF] Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects

This patch adds new subroutine "swaggerize" for Koha::Object and Koha::Objects.

By matching column data types with corresponding Swagger data type, we can e.g.
numify values in order to be able to return borrowernumber:123 instead of
borrowernumber:"123".

Before this patch, we usually returned an unblessed representation of the
Koha-object in controller by calling e.g.
  $c->$cb($patron->unblessed, 200);

This would cause the returned JSON to have { "borrowernumber": "123" ... }, or
in other words everything as strings (apart from undefs, which would be null).

Now, we also return an unblessed representation of the Koha-object, but where
numbers are actually numified simply by calling e.g.
  $c->$cb($patron->swaggerize, 200);

Here our returned JSON will have { "borrowernumber": 123 ... } in "integer" data
type of Swagger, as well as every other column that is defined to be an integer.
Decimals, doubles and floats should have "number" data type and will also be
numified for the returned JSON.

To test:
1. Apply this patch and run t/db_dependent/Koha/Util/Swagger.t
2. Apply REST of the patches and follow their test plan for seeing how this
   works in practice

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Josef Moravec 2016-11-08 21:32:39 UTC
Created attachment 57353 [details] [review]
[SIGNED-OFF] Bug 17008: Fix current data types

This patch changes current Swagger definitions for patrons and holds to have
data types corresponding to column data types in their database tables.

To test:
1. GET http://yourlibrary/api/v1/patrons/YYY where YYY is existing borrowernumber
2. Observe that numbers / integers are in string data type.
3. Apply this patch
4. Repeat step 1.
5. Observe that numbers / integers are now actually numbers / integers.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Josef Moravec 2016-11-08 21:32:48 UTC
Created attachment 57354 [details] [review]
[SIGNED-OFF] Bug 17008: Test to compare Swagger property types against column data types

This patch adds a test that iterates through each definition-object in Swagger
and compares their properties' data types to corresponding Koha-object's column's
data type by using Koha::Util::Swagger::column_type_to_swagger_type().

This test will also check if the column is nullable and will complain
unless property has also "null" defined in its type.

To test:
1. Run t/db_dependent/api/v1/swagger/definitions.t
2. Observe that tests pass (if not, open a Bug to fix issues)
3. Modify api/v1/swagger/definitions/patron.json cardnumber to be type "integer".
4. Run the test again.
5. Observe that test fails and will complain about incorrect data type.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Josef Moravec 2016-11-08 21:32:56 UTC
Created attachment 57355 [details] [review]
Bug 17008: followup - fix cityid type
Comment 24 Tomás Cohen Arazi 2017-01-17 18:04:10 UTC
I'm failing this one:

- Fixing data types and providing and extension for Koha::Object to handle some inconsistencies are different concerns. And bug 17926 provides a more simple way of doing it.
- If you still feel we need to make Koha::Object handle data transformations suitable for output on the REST api, please file a new bug. We still need to openly discuss two things: mistakes in our DB model, and the fact that user facing REST apis don't need to be tied to the underlying model.
- Many of the type fixes are correct, but we need to make booleans be booleans, and this patch fails to do so, making them integers.

I still think we could make use of the column-to-swagger table, but I'm not sure how to deal with Booleans (which are wrongly mapped to integers currently in most of the cases, as there are only a dozen cases in which they are really integers).

I'm open to discuss this on separate bugs that don't catch-them-all. So I'm closing this one, and available to help resurrect the ideas behind this bug.