Description
Lari Taskula
2016-08-01 07:19:39 UTC
Created attachment 54486 [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. For this to succeed, we need a SQL data type => Swagger data type mapping. See "_sql_to_swagger_data_type_mapping" in definitions.t. SQL data types currently defined in the mapping are extracted with a simple script that iterates through each column from each schema's resultset and storing the data type, so the mapping covers at least all currently defined data types and some manually added ones as well. 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. Created attachment 54502 [details] [review] Bug 17008: Test to compare Swagger property types against column data types Created attachment 54508 [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". Created attachment 54509 [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. Created attachment 54510 [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. Run t/db_dependent/api/v1/swagger/definitions.t 2. Note that test has a lot of failures. 3. Apply this patch. 4. Run t/db_dependent/api/v1/swagger/definitions.t 5. Note that tests pass. 6. Run REST of the tests in t/db_dependent/api/v1/* 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 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. 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. 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. Created attachment 56247 [details] [review] Bug 17008: Add "swaggerize" for Koha::Object and Koha::Objects 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. Created attachment 56249 [details] [review] Bug 17008: Test to compare Swagger property types against column data types Rebased on top of master. Created attachment 56250 [details] [review] Bug 17008: Fix current data types Fixed nullability for "branchcode". Created attachment 56251 [details] [review] Bug 17008: Test to compare Swagger property types against column data types 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 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. 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. Removed minifying step from test plans after Bug 17432. 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> 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> 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> Created attachment 57355 [details] [review] Bug 17008: followup - fix cityid type 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. |