fix: Add protocol default value for version and model endpoints & relax supported enum in OpenAPI

[tiopramayudi]

Description

Version endpoints and model endpoints that are deployed before protocol is introduced will have empty string or null value. This will be a problem when new SDK is published, the new SDK has strict validation regarding the enum value and type of each response from server. For empty string value of protocol in old model(version) endpoints, the new SDK will return error since the acceptable value for protocol is UPI_V1 and HTTP_JSON, hence this PR will add default value for protocol to reduce this issue arise in the future.

Also this PR will relax the enum for DeploymentMode and Protocol so it can accept empty string as enum value. This is to reduce chances error during deserialize model API response that containing model endpoints rule that has version_endpoints data with empty protocol or deployment mode. Modifying model endpoint rule by using sql will require complex jsonb query hence for now we just relax the enum value

Modifications

Add database script to update protocol default value for model_endpoints and version_endpoints

Tests

Checklist

• Added PR label
• Added unit test, integration, and/or e2e tests
• Tested locally
• Updated documentation
• Update Swagger spec if the PR introduce API changes
• Regenerated Golang and Python client if the PR introduces API changes

Release Notes

[ghost]

👇 Click on the image for a new way to code review

Legend

[deadlycoconuts]

Super tiny nit but can we have the SQL commands in capital letters for consistency with other migration scripts (and for very slightly better readability) 😅 Thanks! 🙏

[deadlycoconuts]

Actually are we planning to enforce the setting of the protocol field (aka we are no longer accepting non-null values)? If we are already performing this DB migration, and neither the SDK/UI/API accept null values for the protocol field for new models created, then changing the swagger client specs to accept null values will allow users to use null values for the protocol field via the SDK, which isn't what we want right?

I think my point is that performing the DB migration (there's no need to add it to the repo) is enough for all existing models saved without a protocol specified, and we shouldn't modify the swagger specs, since all the newer models already contain a value for that field.

[tiopramayudi]

Actually are we planning to enforce the setting of the protocol field (aka we are no longer accepting non-null values)? If we are already performing this DB migration, and neither the SDK/UI/API accept null values for the protocol field for new models created, then changing the swagger client specs to accept null values will allow users to use null values for the protocol field via the SDK, which isn't what we want right?

I think my point is that performing the db migration is enough for all existing models saved without a protocol specified, and not modify the swagger specs, since all newer models already contain a value for that field.

That's for version_endpoints but for model_endpoints there is rule field that has jsonb type, containing destination rules which including information about version_endpoints where it will cause issue for model endpoint where the version_endpoints is doesn't have protocol or deployment mode information. We can do some migration though using sql, but the query will be complex

[deadlycoconuts]

Ahhh I see I forgot there's another table to update. Hmmm in that case all's good if we manage to migrate the model_endpoints table right? I think we've done something similar for Turing to update a nested jsonb field before; let me try crafting a script in a while.

[tiopramayudi]

Ahhh I see I forgot there's another table to update. Hmmm in that case all's good if we manage to migrate the model_endpoints table right? I think we've done something similar for Turing to update a nested jsonb field before; let me try crafting a script in a while.

Ahhh I see I forgot there's another table to update. Hmmm in that case all's good if we manage to migrate the model_endpoints table right? I think we've done something similar for Turing to update a nested jsonb field before; let me try crafting a script in a while.

Ahhh I see I forgot there's another table to update. Hmmm in that case all's good if we manage to migrate the model_endpoints table right? I think we've done something similar for Turing to update a nested jsonb field before; let me try crafting a script in a while.

🙏 .Yup if the model_endpoints is updated I think we don't need to relax the enum.

[deadlycoconuts]

Okay, after spending time poking into this whole thing, I have a couple of observations and changes to propose:

SDK

As the SDK used to allow null values for the values above, it's certain that some of our users have gotten used to calling certain methods with these values unspecified (null). I'm taking back whatever I said in my posts earlier, but to continue offering this flexibility, we should probably edit the protocol and deployment_mode fields in the OpenAPI specs to allow these values to be nullable (and rely on the API server to set defaults on them).

The other alternative is to not allow null values to be accepted anywhere (including the API server) anymore so that we don't have to deal with such cases anymore. To avoid existing users being interrupted, we can simply ensure that exposed methods on the SDK properly set default values and not allow the SDK to send null values to the API server (by updating the OpenAPI specs).

I'm sitting on the fence for these two options, so I'm okay with either of these really (please share any opinions any of you have on this).

API Server

protocol and deployment_mode are both fields that can be left empty when sent as requests to the API server. However, the way they are processed are kinda different - the API server does not recognise an 'empty' protocol as an actual enum value, though the 'empty' deployment mode is recognised as an actual enum value EmptyDeploymentMode. In both cases, there's no actual 'empty' protocol or deployment as the API server defaults them to HTTP_JSON and serverless respectively.

To keep things clean, I suggest we remove the EmptyDeploymentMode enum in the future to not make it appear as an actual supported deployment mode and instead handle the setting of the default value serverless if the deployment_mode field is empty. (This is pretty low priority and can be done next time).

DB

Both protocol and deployment_mode are stored in a variety ways in the DB. They are either defined, null or are stored as empty strings (those that are null or empty are most probably stored in the DB when the API server was in older versions where these fields were either undefined or accepted empty strings). These are the tables where these values can be found:

• protocol
  • version_endpoints table
    • protocol column
  • model_endpoints table
    • rule column where it can be found via the path 'destinations'->0->'version_endpoint'->>'protocol'
    • protocol column
• deployment_mode
  • version_endpoints table
    • deployment_mode column
  • model_endpoints table
    • deployment_mode column where it can be found via the path 'destinations'->0->'version_endpoint'->>'deployment_mode'

With such a variety of ways these values are stored in the DB, for simplicity and clarity, I suggest that we perform a migration to correct these values to make them defined once and for good. These are the SQL commands we can perform to migrate each of the locations where the values above can be found:

• protocol
  • version_endpoints table

    •  UPDATE version_endpoints SET protocol='HTTP_JSON' WHERE protocol='' OR protocol IS NULL;

  • model_endpoints table

    •  UPDATE model_endpoints SET rule = jsonb_set(rule, '{destinations,0,version_endpoint,protocol}', '"HTTP_JSON"') WHERE (rule->'destinations'->0->'version_endpoint'->>'protocol'='' OR NOT rule->'destinations'->0->'version_endpoint' ? 'protocol') AND rule IS NOT NULL;

    •  UPDATE model_endpoints SET protocol='HTTP_JSON' WHERE protocol='' OR protocol IS NULL;

• deployment_mode
  • version_endpoints table

    •  UPDATE version_endpoints SET deployment_mode='serverless' WHERE deployment_mode='' OR deployment_mode IS NULL;

  • model_endpoints table

    •  UPDATE model_endpoints SET rule = jsonb_set(rule, '{destinations,0,version_endpoint,deployment_mode}', '"serverless"') WHERE (rule->'destinations'->0->'version_endpoint'->>'deployment_mode'='' OR NOT rule->'destinations'->0->'version_endpoint' ? 'deployment_mode') AND rule IS NOT NULL;
      
      

[tiopramayudi]

Okay, after spending time poking into this whole thing, I have a couple of observations and changes to propose:

SDK

As the SDK used to allow null values for the values above, it's certain that some of our users have gotten used to calling certain methods with these values unspecified (null). I'm taking back whatever I said in my posts earlier, but to continue offering this flexibility, we should probably edit the protocol and deployment_mode fields in the OpenAPI specs to allow these values to be nullable (and rely on the API server to set defaults on them).

The other alternative is to not allow null values to be accepted anywhere (including the API server) anymore so that we don't have to deal with such cases anymore. To avoid existing users being interrupted, we can simply ensure that exposed methods on the SDK properly set default values and not allow the SDK to send null values to the API server (by updating the OpenAPI specs).

I'm sitting on the fence for these two options, so I'm okay with either of these really (please share any opinions any of you have on this).

API Server

protocol and deployment_mode are both fields that can be left empty when sent as requests to the API server. However, the way they are processed are kinda different - the API server does not recognise an 'empty' protocol as an actual enum value, though the 'empty' deployment mode is recognised as an actual enum value EmptyDeploymentMode. In both cases, there's no actual 'empty' protocol or deployment as the API server defaults them to HTTP_JSON and serverless respectively.

To keep things clean, I suggest we remove the EmptyDeploymentMode enum in the future to not make it appear as an actual supported deployment mode and instead handle the setting of the default value serverless if the deployment_mode field is empty. (This is pretty low priority and can be done next time).

DB

Both protocol and deployment_mode are stored in a variety ways in the DB. They are either defined, null or are stored as empty strings (those that are null or empty are most probably stored in the DB when the API server was in older versions where these fields were either undefined or accepted empty strings). These are the tables where these values can be found:

• protocol

  • version_endpoints table

    • protocol column

  • model_endpoints table

    • rule column where it can be found via the path 'destinations'->0->'version_endpoint'->>'protocol'
    • protocol column

• deployment_mode

  • version_endpoints table

    • deployment_mode column

  • model_endpoints table

    • deployment_mode column where it can be found via the path 'destinations'->0->'version_endpoint'->>'deployment_mode'

With such a variety of ways these values are stored in the DB, for simplicity and clarity, I suggest that we perform a migration to correct these values to make them defined once and for good. These are the SQL commands we can perform to migrate each of the locations where the values above can be found:

• protocol

  • version_endpoints table

    •  UPDATE version_endpoints SET protocol='HTTP_JSON' WHERE protocol='' OR protocol IS NULL;

  • model_endpoints table

    •  UPDATE model_endpoints SET rule = jsonb_set(rule, '{destinations,0,version_endpoint,protocol}', '"HTTP_JSON"') WHERE (rule->'destinations'->0->'version_endpoint'->>'protocol'='' OR NOT rule->'destinations'->0->'version_endpoint' ? 'protocol') AND rule IS NOT NULL;

    •  UPDATE model_endpoints SET protocol='HTTP_JSON' WHERE protocol='' OR protocol IS NULL;

• deployment_mode

  • version_endpoints table

    •  UPDATE version_endpoints SET deployment_mode='serverless' WHERE deployment_mode='' OR deployment_mode IS NULL;

  • model_endpoints table

    •  UPDATE model_endpoints SET rule = jsonb_set(rule, '{destinations,0,version_endpoint,deployment_mode}', '"serverless"') WHERE (rule->'destinations'->0->'version_endpoint'->>'deployment_mode'='' OR NOT rule->'destinations'->0->'version_endpoint' ? 'deployment_mode') AND rule IS NOT NULL;

Once we update all the records in DB, I think we don't need to change the OpenAPI spec (accepting empty string value as enum). In SDK we need to populate the default if it is not supplied by user ( I think this is already implemented). I think in API server we already implement the default value if the deployment mode or protocol is empty, the reasoning why we still accept empty string value is to maintain backward compatibility for older sdk that doesn't pass those information

[deadlycoconuts]

Once we update all the records in DB, I think we don't need to change the OpenAPI spec (accepting empty string value as enum). In SDK we need to populate the default if it is not supplied by user ( I think this is already implemented). I think in API server we already implement the default value if the deployment mode or protocol is empty, the reasoning why we still accept empty string value is to maintain backward compatibility for older sdk that doesn't pass those information

Sounds good, let's keep the OpenAPI spec as it is then after updating all the records in the DB :) Thanks a lot for sharing your input on this!

[leonlnj]

I dont think we should support "" by default. to confirm. Also i prefer the db_migration to not be part of the repo because this is a one time activity for old models.

So if we do the db migration manually, can i confirm that we dont need this PR?

[tiopramayudi]

I dont think we should support "" by default. to confirm. Also i prefer the db_migration to not be part of the repo because this is a one time activity for old models.

I don't have strong opinion on this, both is okay

So if we do the db migration manually, can i confirm that we dont need this PR?

Yup, correct. I can close this if we are agree to do it manually

[leonlnj]

Yup, correct. I can close this if we are agree to do it manually

Yep, we will do the db migration manually it in the upcoming release. Trying to keep db-migration to what is really needed only, else it will hit 100 scripts soon and become a pain to manage