Another unicode issue in SinglefileData

[giovannipizzi]

In SinglefileData.get_content(), shouldn't we instead return bytes (and fix all places where this method is used, e.g. also in UpfData and the tools reading it?

aiida-core/aiida/orm/nodes/data/singlefile.py

Lines 59 to 65 in dfd616f

	def get_content(self):
	"""Return the content of the single file stored for this data node.
	:return: the content of the file as a string
	"""
	with self.open() as handle:
	return handle.read()

Otherwise it means we are assuming this is a SingleutfstringData

Mentioning @sphuber

[sphuber]

Not sure. The SinglefileData correctly writes the file using binary mode to just copy over bytes wholesale. The open method will by default open in text mode, but not specify any encoding so the default encoding of the OS is used. Is this not what we want? Should we always return byte strings and leave it up to the caller to do something with it. I am not sure if this would not cause unexpected things for users calling these methods, as they are top-level node API methods after all.

[ltalirz]

I would tend to agree with @sphuber here in that get_content is a shorthand for opening the file with default settings and reading - for other use cases there is node.open.
Perhaps it would make sense to document this in the help string?

[giovannipizzi]

I fear that if the file was binary, people will use get_content and this will crash.
Ok to document, and I understand we don't want probably to change the API, but maybe we should have a get_binary_content (or have a flag binary=True), and use it internally? This came out because in the REST API PR, get_content() is used and then encoded in 'utf8' to return a bytestring, and this looks to me convoluted and source of bugs/crashes for binary data

[sphuber]

For the REST API, if they know they want a byte string, they can always replace get_content with:

with node.open(mode='rb') as handle:
    return handle.read()

I think get_content should be a shortcut if we want human readable content. If one knows it can be binary or they want bytes, just use the open method

[greschd]

I have a related question: Is there a better way to copy a SinglefileData out of the repository (via the CLI) than verdi node cat PK filename > new_file? This command currently suffers from the same error, namely

Critical: failed to get the content of file `filename`: 'utf-8' codec can't decode byte 0xac in position 10: invalid start byte

[sphuber]

Not really I am afraid. Some of the sub classes such as CifData have a specific endpoint verdi data cif content but that will most likely from the same problem

[giovannipizzi]

About this last comment:

• Wouldn't it better to have the default as byte stream here for get_object_content?

  aiida-core/aiida/orm/utils/repository.py

  Line 129 in 8456116

  def get_object_content(self, key, mode='r'):

• If we don't think this is the right default, still probably we should pass 'b' as the mode in

  aiida-core/aiida/cmdline/commands/cmd_node.py

  Line 45 in 8456116

  content = node.get_object_content(relative_path)

  and then let click do its job in echo, however passing nl=False as indicated here: https://click.palletsprojects.com/en/7.x/utils/