Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[formrecognizer] model_id param validation #11569

Merged
merged 3 commits into from
May 26, 2020

Conversation

kristapratico
Copy link
Member

Item on arch board issues #11283

Original suggestion was to to enforce that user passes guid for methods that accept model_id parameter. Based on architect guidance we will not validate for this, but will validate that model_id is not None or an empty string.

Reasoning is that service could eventually allow users to name their models with "real" names and not just server generated guids. It's reasonable to check for None/empty string since this will likely never be a valid model_id and prevents a 404 error in the path URL.

@kristapratico kristapratico added Cognitive Services Client This issue points to a problem in the data-plane of the library. FormRecognizer labels May 20, 2020
@kristapratico kristapratico self-assigned this May 20, 2020
@kristapratico
Copy link
Member Author

@maririos FYI since we talked about this

@maririos
Copy link
Member

ohh interesting.
In .NET we do validate that we can convert the string into a GUID, because currently the service expects a UUID. It is worth to checking this in Python?

FYI @samvaity as she was looking into consistency across languages for this item

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but also waiting to hear what @samvaity thinks about guid validation

@samvaity
Copy link
Member

samvaity commented May 21, 2020

In .NET we do validate that we can convert the string into a GUID, because currently the service expects a UUID. It is worth to checking this in Python?

I think we should validate if it is a valid UUID. Especially since the service does not validate that eagerly and will result in a request failure.
We can save users time and cost by failing early on the client-side.
What do you think @kristapratico ?

@@ -277,6 +277,9 @@ def begin_recognize_custom_forms(self, model_id, form, **kwargs):
:caption: Recognize fields and values from a custom form.
"""

if not model_id:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - don't we have match conditions for delete operations in form?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you show me where you see that?

@kristapratico
Copy link
Member Author

I think we should validate if it is a valid UUID. Especially since the service does not validate that eagerly and will result in a request failure.
We can save users time and cost by failing early on the client-side.
What do you think @kristapratico ?

I talked to Johan about this and posted the reasoning above:

Reasoning is that service could eventually allow users to name their models with "real" names and not just server generated guids. It's reasonable to check for None/empty string since this will likely never be a valid model_id and prevents a 404 error in the path URL.

@samvaity
Copy link
Member

I talked to Johan about this and posted the reasoning above:
Reasoning is that service could eventually allow users to name their models with "real" names and not just server generated guids. It's reasonable to check for None/empty string since this will likely never be a valid model_id and prevents a 404 error in the path URL.

Not sure how other languages are handling this currently, but Java will always first convert the user-provided string to a UUID, using fromString() from UUID's lib class to pass it down to the service. If the string provided to fromString() does not conform to UUID specifications it will throw an IllegalArgumentException.

@kristapratico
Copy link
Member Author

Not sure how other languages are handling this currently, but Java will always first convert the user-provided string to a UUID, using fromString() from UUID's lib class to pass it down to the service. If the string provided to fromString() does not conform to UUID specifications it will throw an IllegalArgumentException.

But if the service eventually allows the user to name the model, e.g. "myInvoiceModel", won't this fail with IllegalArgumentException? This is what Johan wants to avoid

@samvaity
Copy link
Member

samvaity commented May 26, 2020

But if the service eventually allows the user to name the model, e.g. "myInvoiceModel", won't this fail with IllegalArgumentException? This is what Johan wants to avoid

To allow this, the service will probably have to start accepting Strings in their API's and that's when we (Java) can plan to remove the UUID.fromString(). Currently, with the service expecting the type UUID, there is no way of escaping converting the user-provided string to UUID.

@kristapratico kristapratico merged commit a8a90d2 into Azure:master May 26, 2020
@kristapratico kristapratico deleted the model-param-validation branch May 26, 2020 19:21
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request May 27, 2020
…into models_to_submodels

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [text analytics] merging feature branch into master (Azure#11632)
  regen swagger and make list_indexes pageable (Azure#11635)
  Sync eng/common directory with azure-sdk-tools repository (Azure#11566)
  Rename to completed_on and requested_on in the CustomFormModel (Azure#11592)
  [formrecognizer] model_id param validation (Azure#11569)
  [Batch] Fix issue in latest REST API (Azure#11604)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Form Recognizer Cognitive Services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants