remove hardcoded encoding

[merla18]

since the client lib (not like the API) requires the document encoding, we should not hardcoded UTF8 by default. Added code used in other sample applications. Tested

[theacodes]

@gguuss can you review?

[theacodes]

My first concern is that this doesn't seem entirely correct. On Python 2.7, the strings we get from the command line are bytes and we manually decode those to utf-8. In the Python 2 case, we're passing the wrong encoding to the API.

[monattar]

Note that what is sent to the API is not the encoding of the input text, but the encoding used to calculate offsets in the response.

[theacodes]

@monattar I think my concern still holds true. In Python 2.7, since we're decoding bytes from the input into utf-8, we should specify utf-8 as the encoding to the API.

[gguuss]

+1 to @jonparrott's comments, additionally, I'm concerned of the following:

• This code adds complexity without adding any actual value for most developers
• This code introduces a subroutine that will not appear in the how-to documentation
• Please add tests for the new code you're introducing highlighting the issue you're fixing

[gguuss]

For better snippets, please update this to be an input to the function as opposed to something that will not appear in devsite.

[gguuss]

@monattar Furthermore, what is broken and how does this fix it?

[gguuss]

Why aren't you using the enumeration type?

[monattar]

I think we need a test to clarify. Could we please add a test for this content "foo→bar" and check the offset of "bar"?

[gguuss]

FYI missing newline

[merla18]

here is the results of the test:

• if I use the original " enums.EncodingType.UTF8" in the code and print the char at the offset of the original text

$ python snippets.py sentiment-entities-text "foo→bar"
Mentions:
Name: "bar"
Begin Offset : 6
Content : bar
original text char at offset : r

• I use get_native_encoding_type() and print the char at the offset of the original text:
  $ python snippets.py sentiment-entities-text "foo→bar"
  Mentions:
  Name: "bar"
  Begin Offset : 4
  Content : bar
  original text char at offset : b

The change get_native_encoding_type() seems to return the correct answer.

[gguuss]

@merla18 Thanks for the usage example, helps to clarify what this fixes.

So that other contributors don't reintroduce the bug, you should add a unit test to language/cloud-client/v1beta2/snippets_test.py.

[monattar]

Also wanted to mention that encoding is one of the trickiest things to get right, and we're trying to push that into the client library. So, hopefully specifying the EncodingType wouldn't be necessary at all in the future.

[gguuss]

Any reason UTF8 isn't here? Below, the input is explicitly encoded as UTF8.

[monattar]

This is identified what Python natively does, regardless of any explicit encoding/decoding.

[theacodes]

@merla18@monattar there's some weirdness going on here.

See my annotated code here:

# Regardless of if the input is bytes or unicode, this will ensure that# it is unicode (utf-8 in Python 2, 16 or 32 in Python 3).ifisinstance(text, six.binary_type):
text=text.decode('utf-8')
# This ensures that the unicode string is encoded as utf-8 regardless# of original encoding. encode returns# bytes, but protobuf will automatically re-encode to utf-8 for it's# wire format.document.content=text.encode('utf-8')
document.type=enums.Document.Type.PLAIN_TEXT# At this point, document is (asciipb):# type: PLAIN_TEXT# content: "foo\342\206\222bar"## Or in wire format:# b'\x08\x01\x12\tfoo\xe2\x86\x92bar'# Which is *clearly* utf-8.

So we're always sending utf-8 on the wire. Protobuf demands that we do so, we can't set a utf-16 bytestring to the message:

>>>document.content=u"foo→bar".encode('utf-16')
***ValueError: b'\xff\xfef\x00o\x00o\x00\x92!b\x00a\x00r\x00'hastypebytes, butisn'tvalidUTF-8encoding. Non-UTF-8stringsmustbeconvertedtounicodeobjectsbeforebeingadded.

So things get weird now. If we make the request and specify UTF8 as the encoding (which is correct) we get the wrong offset:

>>>language_client.analyze_entity_sentiment(document, enums.EncodingType.UTF8)
entities {
name: "bar"type: LOCATIONsalience: 1.0mentions {
text {
content: "bar"begin_offset: 6
}
type: COMMONsentiment {
}
}
sentiment {
}
}
language: "en"

The offset is 6 when clearly the offset is 4 in the utf-8 string.

What's even weirder is that if we pass in UTF16 as the encoding:

>>>language_client.analyze_entity_sentiment(document, enums.EncodingType.UTF16)
entities {
name: "bar"type: LOCATIONsalience: 1.0mentions {
text {
content: "bar"begin_offset: 4
}
type: COMMONsentiment {
}
}
sentiment {
}
}
language: "en"

We get the correct offset for utf-8 but the wrong offset for utf-16. In fact, the offsets seem to be switched!

Any ideas?

[gguuss]

@monattar@merla18 I'm seeing the same behavior in Java:

 public List<Entity> entitySentimentText(String text) throws IOException {
Document doc = Document.newBuilder()
.setContent(new String(java.nio.charset.Charset.forName("UTF-8").encode(text).array()))
.setType(Type.PLAIN_TEXT).build();
AnalyzeEntitySentimentRequest request = AnalyzeEntitySentimentRequest.newBuilder()
.setDocument(doc)
.setEncodingType(EncodingType.UTF8).build();
AnalyzeEntitySentimentResponse response = languageApi.analyzeEntitySentiment(request);
return response.getEntitiesList();
}

With an input of "1234♥♥♥Google." shows the offset of Google as 13. Changing to UTF-16, e.g.

 public List<Entity> entitySentimentText(String text) throws IOException {
Document doc = Document.newBuilder()
.setContent(new String(java.nio.charset.Charset.forName("UTF-16").encode(text).array()))
.setType(Type.PLAIN_TEXT).build();
AnalyzeEntitySentimentRequest request = AnalyzeEntitySentimentRequest.newBuilder()
.setDocument(doc)
.setEncodingType(EncodingType.UTF8).build();
AnalyzeEntitySentimentResponse response = languageApi.analyzeEntitySentiment(request);
return response.getEntitiesList();
}

Shows the offset of Google as 21.

Conversely, passing the UTF8 string and telling the API that it's UTF-16 shows the correct offset, 7.

This could be a bug that is outside of the scope of the samples and client libraries.

[gguuss]

FYI, added in this PR with test, snippet, and style fixes.

[theacodes]

Closing in favor of #961