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

Check if StorageInputType needs to be in TranslationSource or in DocumentTranslationInput #23084

Closed
maririos opened this issue Aug 3, 2021 · 5 comments
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Translator
Milestone

Comments

@maririos
Copy link
Member

maririos commented Aug 3, 2021

The reason why it is in DocumentTranslationInput is that both source and target had to have the same storageInput. I believe the service updated their behavior and this is no longer required, but need to double-check.

If it is no longer needed for a target too, we should move the DocumentTranslationInput.Storagetype property to the TranslationSource type

@maririos maririos added Client This issue points to a problem in the data-plane of the library. Cognitive - Translator labels Aug 3, 2021
@maririos maririos added this to the [2021] September milestone Aug 3, 2021
@ZulaMostafa
Copy link
Contributor

It's true now that source and target can have different storageInput. But I don't get why should we move the property to TranslationSource, shouldn't the target have a StorageType too?

@maririos
Copy link
Member Author

maririos commented Aug 9, 2021

Ohh interesting. So looking at the swagger, StorageType seems to be independent of source or target (which is how we have it in the library).
How will a request work in order to say the source is a folder, and the target is a file? or the opposite, source is file and target is a folder?

Something that could help will be to have a table with the 4 possible combinations and see which cases works, which cases don't work, and try to send those requests with Postman and see what you get

@ZulaMostafa
Copy link
Contributor

Let's have a look into the interesting results, I've tested all the combinations using both Postman and the actual SDK.

Case 1:
Source: Folder
Target: Folder
Status: Succeeds

Case 2:
Source: File
Target: Folder
Status: Succeeds only if the StorageType is specified as File.

Case 3:
Source: File
Target: File
Status: Succeeds only if the StorageType is specified as File.
There is one more comment about this case, it works only if the target is a container with adding filename at the end of the path in the URL. If the target is a blob Postman responds with Ok status but the file never changes. and for the SDK, that was the error I got:
Cannot access target document location with the current permissions.
Status: 200 (OK)
ErrorCode: InvalidTargetDocumentAccessLevel

Case 4:
Source: Folder
Target: File
Status: Fails.
When I use a container as the target with adding the file name at the end of the path it creates a new folder. And when i use a blob it's just the same as the previous case.

I think we need to move StorageType property to TranslationSource. I had to specify the type only if the source was File. Regarding the permissions issue, I'm not sure if I've done something wrong but the permissions given to the blob was read and write.

@maririos
Copy link
Member Author

This is great @ZulaMostafa . I agree with you that it seems like we can move the SotrageType property into TranslationSource.
There is a concern and is that the service could decide to change this in the future and we will have to add extra logic to see how we reconcile.
Let's ask the service team and get their opinion.

@maririos maririos added the blocking-release Blocks release label Aug 16, 2021
@maririos maririos modified the milestones: [2021] September, Backlog Aug 30, 2021
@maririos maririos removed the blocking-release Blocks release label Aug 30, 2021
@maririos
Copy link
Member Author

The service team didn't see a reason to move it, especially since the service already GAd this way. The fear is that in the future this property applies equally to both source and target and if we move it in the SDK to only source, it will be harder to reconcile.

Decision in SDK: leave as is

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Translator
Projects
None yet
Development

No branches or pull requests

2 participants