From 05be3e60fa59fd87af0bd4d65cbd265ebbca8dd9 Mon Sep 17 00:00:00 2001
From: Valentin Lorentz <vlorentz@softwareheritage.org>
Date: Wed, 18 Dec 2019 18:32:27 +0100
Subject: [PATCH] Clean temporary file handling for metadata generation, and
add test.
This uses a context-manager instead of manually handling
removing the file.
---
swh/deposit/cli/client.py | 141 +++++++++++----------------
swh/deposit/tests/cli/test_client.py | 47 +++++++--
2 files changed, 96 insertions(+), 92 deletions(-)
diff --git a/swh/deposit/cli/client.py b/swh/deposit/cli/client.py
index 9eb7bfc3..f631e165 100644
--- a/swh/deposit/cli/client.py
+++ b/swh/deposit/cli/client.py
@@ -49,7 +49,7 @@ def _url(url):
return url
-def generate_metadata_file(name, external_id, authors):
+def generate_metadata_file(name, external_id, authors, temp_dir):
"""Generate a temporary metadata file with the minimum required metadata
This generates a xml file in a temporary location and returns the
@@ -67,8 +67,7 @@ def generate_metadata_file(name, external_id, authors):
Filepath to the metadata generated file
"""
- _, tmpfile = tempfile.mkstemp(prefix='swh.deposit.cli.')
-
+ path = os.path.join(temp_dir, 'metadata.xml')
# generate a metadata file with the minimum required metadata
codemetadata = {
'entry': {
@@ -82,29 +81,13 @@ def generate_metadata_file(name, external_id, authors):
},
}
- logging.debug('Temporary file: %s', tmpfile)
+ logging.debug('Temporary file: %s', path)
logging.debug('Metadata dict to generate as xml: %s', codemetadata)
s = xmltodict.unparse(codemetadata, pretty=True)
logging.debug('Metadata dict as xml generated: %s', s)
- with open(tmpfile, 'w') as fp:
+ with open(path, 'w') as fp:
fp.write(s)
- return tmpfile
-
-
-def _cleanup_tempfile(config):
- """Clean up the temporary metadata file generated.
-
- Args:
-
- config (Dict): A configuration dict with 2 important keys for
- that routine, 'cleanup_tempfile' (bool) and 'metadata' (path
- to eventually clean up)
-
- """
- if config['cleanup_tempfile']:
- path = config['metadata']
- if os.path.exists(path):
- os.unlink(path)
+ return path
def _client(url, username, password):
@@ -144,7 +127,7 @@ def client_command_parse_input(
username, password, archive, metadata,
archive_deposit, metadata_deposit,
collection, slug, partial, deposit_id, replace,
- url, name, authors):
+ url, name, authors, temp_dir):
"""Parse the client subcommand options and make sure the combination
is acceptable*. If not, an InputError exception is raised
explaining the issue.
@@ -187,66 +170,55 @@ def client_command_parse_input(
'deposit_id': optional deposit identifier
"""
- cleanup_tempfile = False
+ if archive_deposit and metadata_deposit:
+ # too many flags use, remove redundant ones (-> multipart deposit)
+ archive_deposit = False
+ metadata_deposit = False
- try:
- if archive_deposit and metadata_deposit:
- # too many flags use, remove redundant ones (-> multipart deposit)
- archive_deposit = False
- metadata_deposit = False
+ if not slug: # generate one as this is mandatory
+ slug = generate_slug()
- if not slug: # generate one as this is mandatory
- slug = generate_slug()
+ if not metadata and name and authors:
+ metadata = generate_metadata_file(name, slug, authors, temp_dir)
- if not metadata and name and authors:
- metadata = generate_metadata_file(name, slug, authors)
- cleanup_tempfile = True
+ if metadata_deposit:
+ archive = None
- if metadata_deposit:
- archive = None
+ if archive_deposit:
+ metadata = None
- if archive_deposit:
- metadata = None
+ if metadata_deposit and not metadata:
+ raise InputError(
+ "Metadata deposit must be provided for metadata "
+ "deposit (either a filepath or --name and --author)")
- if metadata_deposit and not metadata:
- raise InputError(
- "Metadata deposit must be provided for metadata "
- "deposit (either a filepath or --name and --author)")
+ if not archive and not metadata and partial:
+ raise InputError(
+ 'Please provide an actionable command. See --help for more '
+ 'information')
- if not archive and not metadata:
- raise InputError(
- 'Please provide an actionable command. See --help for more '
- 'information')
+ if replace and not deposit_id:
+ raise InputError(
+ 'To update an existing deposit, you must provide its id')
- if replace and not deposit_id:
- raise InputError(
- 'To update an existing deposit, you must provide its id')
-
- client = _client(url, username, password)
+ client = _client(url, username, password)
- if not collection:
- collection = _collection(client)
+ if not collection:
+ collection = _collection(client)
- return {
- 'archive': archive,
- 'username': username,
- 'password': password,
- 'metadata': metadata,
- 'cleanup_tempfile': cleanup_tempfile,
- 'collection': collection,
- 'slug': slug,
- 'in_progress': partial,
- 'client': client,
- 'url': url,
- 'deposit_id': deposit_id,
- 'replace': replace,
- }
- except Exception: # to be clean, cleanup prior to raise
- _cleanup_tempfile({
- 'cleanup_tempfile': cleanup_tempfile,
- 'metadata': metadata
- })
- raise
+ return {
+ 'archive': archive,
+ 'username': username,
+ 'password': password,
+ 'metadata': metadata,
+ 'collection': collection,
+ 'slug': slug,
+ 'in_progress': partial,
+ 'client': client,
+ 'url': url,
+ 'deposit_id': deposit_id,
+ 'replace': replace,
+ }
def _subdict(d, keys):
@@ -329,17 +301,17 @@ https://docs.softwareheritage.org/devel/swh-deposit/getting-started.html.
url = _url(url)
config = {}
- try:
- logger.debug('Parsing cli options')
- config = client_command_parse_input(
- username, password, archive, metadata, archive_deposit,
- metadata_deposit, collection, slug, partial, deposit_id,
- replace, url, name, author)
- except InputError as e:
- logger.error('Problem during parsing options: %s', e)
- sys.exit(1)
+ with tempfile.TemporaryDirectory() as temp_dir:
+ try:
+ logger.debug('Parsing cli options')
+ config = client_command_parse_input(
+ username, password, archive, metadata, archive_deposit,
+ metadata_deposit, collection, slug, partial, deposit_id,
+ replace, url, name, author, temp_dir)
+ except InputError as e:
+ logger.error('Problem during parsing options: %s', e)
+ sys.exit(1)
- try:
if verbose:
logger.info("Parsed configuration: %s" % (
config, ))
@@ -353,9 +325,6 @@ https://docs.softwareheritage.org/devel/swh-deposit/getting-started.html.
logger.info(r)
- finally:
- _cleanup_tempfile(config)
-
@deposit.command()
@click.option('--url', default='https://deposit.softwareheritage.org',
diff --git a/swh/deposit/tests/cli/test_client.py b/swh/deposit/tests/cli/test_client.py
index b2dafab5..c265e11c 100644
--- a/swh/deposit/tests/cli/test_client.py
+++ b/swh/deposit/tests/cli/test_client.py
@@ -3,6 +3,7 @@
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
+import contextlib
import logging
import os
from unittest.mock import MagicMock
@@ -76,11 +77,15 @@ def test_collection_ok():
def test_single_minimal_deposit(
- sample_archive, mocker, caplog, client_mock, slug):
+ sample_archive, mocker, caplog, client_mock, slug, tmp_path):
""" from:
https://docs.softwareheritage.org/devel/swh-deposit/getting-started.html#single-deposit
""" # noqa
+ metadata_path = os.path.join(tmp_path, 'metadata.xml')
+ mocker.patch('swh.deposit.cli.client.tempfile.TemporaryDirectory',
+ return_value=contextlib.nullcontext(str(tmp_path)))
+
runner = CliRunner()
result = runner.invoke(cli, [
'upload',
@@ -89,6 +94,7 @@ def test_single_minimal_deposit(
'--password', TEST_USER['password'],
'--name', 'test-project',
'--archive', sample_archive['path'],
+ '--author', 'Jane Doe',
])
assert result.exit_code == 0, result.output
@@ -99,18 +105,34 @@ def test_single_minimal_deposit(
client_mock.deposit_create.assert_called_once_with(
archive=sample_archive['path'],
- collection='softcol', in_progress=False, metadata=None,
+ collection='softcol', in_progress=False, metadata=metadata_path,
slug=slug)
-
-def test_single_deposit_slug_collection(
- sample_archive, mocker, caplog, client_mock):
+ with open(metadata_path) as fd:
+ assert fd.read() == f'''\
+<?xml version="1.0" encoding="utf-8"?>
+<entry xmlns="http://www.w3.org/2005/Atom" \
+xmlns:codemeta="https://doi.org/10.5063/SCHEMA/CODEMETA-2.0">
+\t<codemeta:name>test-project</codemeta:name>
+\t<codemeta:identifier>{slug}</codemeta:identifier>
+\t<codemeta:author>
+\t\t<codemeta:name>Jane Doe</codemeta:name>
+\t</codemeta:author>
+</entry>'''
+
+
+def test_single_deposit_slug_generation(
+ sample_archive, mocker, caplog, tmp_path, client_mock):
""" from:
https://docs.softwareheritage.org/devel/swh-deposit/getting-started.html#single-deposit
""" # noqa
slug = 'my-slug'
collection = 'my-collection'
+ metadata_path = os.path.join(tmp_path, 'metadata.xml')
+ mocker.patch('swh.deposit.cli.client.tempfile.TemporaryDirectory',
+ return_value=contextlib.nullcontext(str(tmp_path)))
+
runner = CliRunner()
result = runner.invoke(cli, [
'upload',
@@ -121,6 +143,7 @@ def test_single_deposit_slug_collection(
'--archive', sample_archive['path'],
'--slug', slug,
'--collection', collection,
+ '--author', 'Jane Doe',
])
assert result.exit_code == 0, result.output
@@ -131,9 +154,21 @@ def test_single_deposit_slug_collection(
client_mock.deposit_create.assert_called_once_with(
archive=sample_archive['path'],
- collection=collection, in_progress=False, metadata=None,
+ collection=collection, in_progress=False, metadata=metadata_path,
slug=slug)
+ with open(metadata_path) as fd:
+ assert fd.read() == '''\
+<?xml version="1.0" encoding="utf-8"?>
+<entry xmlns="http://www.w3.org/2005/Atom" \
+xmlns:codemeta="https://doi.org/10.5063/SCHEMA/CODEMETA-2.0">
+\t<codemeta:name>test-project</codemeta:name>
+\t<codemeta:identifier>my-slug</codemeta:identifier>
+\t<codemeta:author>
+\t\t<codemeta:name>Jane Doe</codemeta:name>
+\t</codemeta:author>
+</entry>'''
+
def test_multisteps_deposit(
sample_archive, atom_dataset, mocker, caplog, datadir,
--
GitLab