Skip to content

[TA] Changes based on V3.0 swagger, etc#10593

Merged
mssfang merged 21 commits intoAzure:masterfrom
mssfang:TA-Regenerate
May 9, 2020
Merged

[TA] Changes based on V3.0 swagger, etc#10593
mssfang merged 21 commits intoAzure:masterfrom
mssfang:TA-Regenerate

Conversation

@mssfang
Copy link
Contributor

@mssfang mssfang commented Apr 29, 2020

  • Use V3.0 swagger code base, fixes: [TA] Regenerate code base by using V3.0 swagger #10585, fixes: [TA] update TextAnalyticsServiceVersion when service GA. #8480
  • Hide public constructor of analyzed result, which is no longer an issue. We will keep constructor design, fixes: [TA] Remove output type public constructors #10346
  • Renaming:
    • DetectedLanguage: getScore() to getConfidenceScore()
    • CategorizedEntity: getSubCategory() to getSubcategory()
    • LinkedEntity: getLinkedEntityMatches() to getMatches()
    • DocumentResult to TextAnalyticsResult
    • TextAnalyticsException and TextAnalyticsError: getCode() to getErrorCode()
    • TextAnalyticsWarning: getCode() to getWarningCode()
  • Internal transfer TextAnalyticsErrorException to HttpResponseException and added tests
  • Support not throws exception directly in asycn client but return it as monoError or fluxError. Added class DocumentInputAsyncTest to support it
  • Update sync client to make sure the client throws exception.
  • Added one new method getText() to SentenceSentiment
  • Added a new class, TextAnalyticsWarning and one expandatable enum WarningCode
  • Deprecated TextDocumentInput(String id, String text, String language) constructor, but added setLanguage() setter since language is optional.
  • Remove graphme properties: removed getGraphemeLength() and getGraphemeOffset() from CategorizedEntity, SentenceSentiment, and LinkedEntityMatch. getGraphemeCount() in TextDocumentStatistics has been renamed to getCharacterCount(). fixes: [TA] Encoding and removing grapheme prefix #10587
  • Warningsproperty added to each document-level response object returned from the endpoints. It is a list ofTextAnalyticsWarnings`. fixes: [TA] Warnings is enable in V3.0. Add it to single and batch APIs #10586
  • Added CategorizedEntityCollection, KeyPhrasesCollection, LinkedEntityCollection for having getWarnings() to retrieve warnings.
  • Added a new enum value ADDRESS to EntityCategory.
  • Replaced all single input asynchronous APIs, e.x.,
    • TextAnalyticsPagedFlux<CategorizedEntity> recognizeEntities(String document) to Mono<CategorizedEntityCollection> recognizeEntities(String document).
    • TextAnalyticsPagedFlux<LinkedEntity> recognizeLinkedEntities(String document) to Mono<LinkedEntityCollection> recognizeLinkedEntities(String document).
    • TextAnalyticsPagedFlux<String> extractKeyPhrases(String document) to Mono<KeyPhrasesCollection> extractKeyPhrases(String document).
  • Replaced all single input synchronous APIs, e.x.,
    • TextAnalyticsPagedIterable<CategorizedEntity> recognizeEntities(String document) to CategorizedEntityCollection recognizeEntities(String document).
    • TextAnalyticsPagedIterable<LinkedEntity> recognizeLinkedEntities(String document) to LinkedEntityCollection recognizeLinkedEntities(String document).
    • TextAnalyticsPagedIterable<String> extractKeyPhrases(String document) to KeyPhrasesCollection extractKeyPhrases(String document).

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. TextAnalytics labels Apr 29, 2020
@mssfang mssfang added this to the [2020] May milestone Apr 29, 2020
@mssfang mssfang self-assigned this Apr 29, 2020
@mssfang mssfang changed the title [TA] Changes based on V3.0 swagger, Hide pub [TA] Changes based on V3.0 swagger, etc Apr 29, 2020
@mssfang mssfang requested a review from samvaity April 30, 2020 07:27
@mssfang mssfang marked this pull request as ready for review April 30, 2020 07:28
@mssfang mssfang removed the request for review from anuchandy May 7, 2020 23:49
Copy link
Member

@samvaity samvaity left a comment

Choose a reason for hiding this comment

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

Left a few comments, but rest LGTM.

public static HttpResponse getMockHttpResponse(SimpleResponse<?> response) {
return new HttpResponse(response.getRequest()) {
@Override
public int getStatusCode() {
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected to only return the status code in the response?

Copy link
Member

Choose a reason for hiding this comment

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

The exception the user will get should be the same as if the service returns a status = 400 and all the information with it.

For .net, for example, we have status, error code, and error message and we throw a RequestFailedException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is HttpResponse implementation class. Unless we know what is the expected header values and etc. We can make it up. But the for error message, it exist in the HttpResponseException level.

Objects.requireNonNull(documents, "'documents' cannot be null.");
final Iterator<?> iterator = documents.iterator();
if (!iterator.hasNext()) {
throw new IllegalArgumentException("'documents' cannot be empty.");
Copy link
Member

Choose a reason for hiding this comment

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

This is still not updated.


<T> T clientSetup(Function<HttpPipeline, T> clientBuilder) {
TokenCredential credential = null;
AzureKeyCredential credential = null;
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason we changed this from AAD to AzureKeyCredential?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the new endpoint, is only support Key Credential, it is owned by the service team. We are just using it temporately.

"networkCallRecords" : [ {
"Method" : "POST",
"Uri" : "https://javatextanalyticstestresources.cognitiveservices.azure.com/text/analytics/v3.0-preview.1/entities/linking",
"Uri" : "https://westus2.ppe.cognitiveservices.azure.com/text/analytics/v3.0//entities/linking",
Copy link
Member

Choose a reason for hiding this comment

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

extra / .
Also, do we need to update all the recordings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extra / should be fine.

There is a lot of place involving renaming, such as, score to confidenceScore

public TextDocumentInput(String id, String text) {
this.id = id;
this.text = text;
this.language = "en";
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I was wrong when I suggested this, I didn't realize that this TextDocumentInput constructor didn't take language. I'm confused though: it looks like in master you had two constructors: one that took language, one that did not.

I think you should still have two constructors here like in master. For the one that doesn't take language as a parameter, it should set this.language = "en", since that is the default. Otherwise, this.language = language

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if in the future, the default lanauge is change to 'es. we need to update it to es`. Since service team take default value in service side, we should not add what they are maintaining here. So keep no-assigned in SDK should be an better idea.

@mssfang mssfang requested review from iscai-msft and maririos May 8, 2020 22:56
* @return The object {@link TextDocumentInput} itself.
*/
public void setLanguage(String language) {
public TextDocumentInput setLanguage(String language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure i understand the context on just making setLanguage a setter and not adding it to a separate constructor like you used to have it. Is there a reason why you can't add this constructor back like it is in master:

I really don't think it's a good idea to force users to call TextDocumentInput.setLanguage("en") for the most basic version of TextDocumentInput (you're doing this in your tests). What I think you should do is have two constructors exactly how it is in master. Is there a reason why you're changing this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only time constructor needed is when the parameter is required. But since language is optional. it should not go to constructor. Let's say maybe in the future, more parameter will be added to this class, we will not add another constructor if the value is not required.

@mssfang mssfang requested a review from iscai-msft May 9, 2020 01:12
@mssfang mssfang merged commit 62005e7 into Azure:master May 9, 2020
@mssfang mssfang deleted the TA-Regenerate branch May 9, 2020 04:45
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 - Text Analytics

Projects

None yet

5 participants

Comments