search

unee-t / bz-database

Scripts and schema for the bz database so we can build the BZ FE
GNU Affero General Public License v3.0
0 stars 2 forks source link

Use MySQL `JSON` functions to generate JSON payload (instead of `CONCAT`) #118

Closed kaihendry closed 5 years ago

kaihendry commented 5 years ago

https://dev.mysql.com/doc/refman/5.7/en/json-creation-functions.html#function_json-quote

This will https://github.com/unee-t/bz-database/blob/schema_v4.32.0_lambda_improvements/db%20scripts%20to%20add%20objects%20to%20the%20schema/Add_all_lambda_related_objects_v4.31.sql#L77-L90 fail if there any quotes. Once the schema is updated, it will probably fix https://github.com/unee-t/bz-database/issues/110#issuecomment-469948603. :crossed_fingers:

franck-boullier commented 5 years ago

Thanks for looking into that @kaihendry, I'll implement that ASAP.

franck-boullier commented 5 years ago

This has been implemented in the DEV/Staging and in the DEMO envo. @kaihendry can you check if:

Thanks

franck-boullier commented 5 years ago

This has been implemented in the DEV/Staging and in the DEMO envo.

We had to roll back this change as this apparently created invalid JSON messages...

@kaihendry do we still need to explore the use of JSON_QUOTE or are we using a different strategy?

kaihendry commented 5 years ago

Can you show me your schema diff? Maybe you should be using JSON_OBJECT

echo "CALL mysql.lambda_async( 'arn:aws:lambda:ap-southeast-1:$(acc $STAGE):function:alambda_simple', JSON_OBJECT('id', 87, 'name', 'carrot') );" | mysql -h $(domain $STAGE) -P 3306 -u bugzilla --password=$(ssm MYSQL_PASSWORD)

franck-boullier commented 5 years ago

JSON_OBJECT looks promising indeed: today I'm using CONCAT and it looks like JSON_OBJECT can be a better substitute of that...

Current way to create the Payload:

CONCAT ('{ ' , '"notification_type": "', notification_type , '", "bz_source_table": "', bz_source_table , '", "notification_id": "', notification_id , '", "created_datetime" : "', created_datetime , '", "unit_id" : "', unit_id , '", "case_id" : "', case_id , '", "case_title" : "', case_title , '", "invitor_user_id" : "', invitor_user_id , '", "case_reporter_user_id" : "', case_reporter_user_id , '", "old_case_assignee_user_id" : "', old_case_assignee_user_id , '", "new_case_assignee_user_id" : "', new_case_assignee_user_id , '", "current_list_of_invitees" : "', current_list_of_invitees , '", "current_status" : "', current_status , '", "current_resolution" : "', current_resolution , '", "current_severity" : "', current_severity , '"}' )

Alternative that I'd like to try:

JSON_OBJECT ('notification_type' , notification_type , 'bz_source_table', bz_source_table , 'notification_id', notification_id , 'created_datetime', created_datetime , 'unit_id', unit_id , 'case_id', case_id , 'case_title', case_title , 'invitor_user_id', invitor_user_id , 'case_reporter_user_id', case_reporter_user_id , 'old_case_assignee_user_id', old_case_assignee_user_id , 'new_case_assignee_user_id', new_case_assignee_user_id , 'current_list_of_invitees', current_list_of_invitees , 'current_status', current_status , 'current_resolution', current_resolution , 'current_severity', current_severity )

franck-boullier commented 5 years ago

@kaihendry I've updated the code to generate the payload to use JSON_OBJECT instead of CONCAT in the DEV/Staging and in the DEMO envos. Can you check if this creates any issues with the lambda and payloads?

franck-boullier commented 5 years ago

A few Gotchas: You need to use MySQL 5.7.22 or later OR Maria DB 10.2.3 or later: these are the first version to support JSON commands.

franck-boullier commented 5 years ago

This doesn't seem to work as intended...

In the MEFE:

Using JSON_OBJECT

Update the DB schema to use the JSON_OBJECT method to generate the payload for the lambda notifications. It is NOT possible to add a new message to a case with the MEFE

image

Using CONCAT

Reverting back to the CONCAT way of generating the payload for the lambda notifications. This fixed the issue in the MEFE.

In the BZFE:

Using JSON_OBJECT

Update the DB schema again to use the JSON_OBJECT method to generate the payload for the lambda notifications. This works as intended: the message is added to the case

image

The newly added message created in the BZFE is also visible in the MEFE (as expected) image

@nbiton it seems that there is an issue the MEFE since the new SQL way to generate the payload works well in the BZFE but doesn't work in the MEFE...

kaihendry commented 5 years ago

Works nicely on a quick test. https://s.natalian.org/2019-03-06/1551876416_2560x1440.png

nbiton commented 5 years ago

It must be breaking the BZ API way of adding a comment. MEFE relies on the BZ API to work properly

nbiton commented 5 years ago

You can test it in a MEFE-bypassing way by using the "Post a new comment to a case" saved request in Postman

kaihendry commented 5 years ago

I don't think the CONCAT is needed, it's still in the schema, e.g.:

CALL mysql.lambda_async(CONCAT('arn:aws:lambda:ap-southeast-1:915001051872:function:alambda_simple')

I think we should push this JSON_OBJECT change out to production, since I've only ever seen those 5xx issues in prod https://github.com/unee-t/bz-database/issues/110#issuecomment-469567153

franck-boullier commented 5 years ago

I don't think the CONCAT is needed

It is still needed

I think we should push this JSON_OBJECT change out to production

OK noted. I'm working on that now, I want to also add the fix for issue #114 as part of the schema change and I'm almost there...

franck-boullier commented 5 years ago

Schema v4.32 is a fix for that.