Page MenuHomeSoftware Heritage
Differential D283

Add url validation
ClosedPublic
Actions

Authored by moranegg on Jan 9 2018, 2:15 PM.

Details

Reviewers
ardumont
Maniphest Tasks
T868: Validate Url and create origin from given url for a deposit
Commits
rDDEP62cd26d16b97: Create origin from given url in metadata
rDDEP0dca4092d522: Amend checks with common function _get_metadata
rDDEP48eda313e9d4: Add url validation
Summary

Cheking compatibility between client url and the deposit's metadata entry url.
This will enable the usage of url for origin creation.

Resolves T868

Diff Detail

Repository
rDDEP Push deposit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1145
Build 1489: arc lint + arc unit

Event Timeline

moranegg created this revision.Jan 9 2018, 2:15 PM
Harbormaster completed remote builds in B1144: Diff 948.Jan 9 2018, 2:15 PM
moranegg edited the summary of this revision. (Show Details)Jan 9 2018, 2:16 PM
moranegg added a task: T868: Validate Url and create origin from given url for a deposit.
ardumont requested changes to this revision.Jan 9 2018, 2:43 PM
ardumont added inline comments.
swh/deposit/api/private/deposit_check.py
82

We don't want to expose the implementation detail of how we do the check in the documentation ;)
We want to expose the intent.

A better docstring than my previous attempt would be:

Given a deposit, check its associated metadata for mandatory field presence.
   ...
101

I forgot (dict) instead of (), can you please add it?

119

Please, see comments line 143 below

125

Simply returning the result here would be fine.

Also a docstring would be good.

As a note, there is a typo in the variable's name ;)

142

I don't like setting variable outside of the __init__ construct, especially when we can avoid it :)
(I know we did it in other modules but for other reasons).

If we want to manipulate the metadata multiple times (as introduced here), we should have a method for it. Then the multiple check methods will manipulate those metadata. The intents are clearer that way.

So here, that would give:

  • _check_metadata stays as before (it only checks the metadata for field presence)
  • _check_deposit_metadata becomes just a metadata_get (it's in charge of aggregating metadata)
  • _check_url is almost like you propose but it's in charge of filtering the url fields from the metadata passed as parameter prior to the current proposed check.

Then no need for that intermediary variable.

This revision now requires changes to proceed.Jan 9 2018, 2:43 PM
moranegg updated this revision to Diff 949.Jan 9 2018, 2:44 PM
  • Create origin from given url in metadata
Harbormaster completed remote builds in B1145: Diff 949.Jan 9 2018, 2:44 PM
moranegg marked 5 inline comments as done.Jan 9 2018, 2:58 PM
moranegg updated this revision to Diff 950.Jan 9 2018, 2:59 PM
  • Amend checks with common function _get_metadata
Harbormaster completed remote builds in B1146: Diff 950.Jan 9 2018, 2:59 PM
moranegg updated this revision to Diff 951.Jan 9 2018, 3:05 PM
  • Amend checks with common function _get_metadata
Harbormaster completed remote builds in B1147: Diff 951.Jan 9 2018, 3:05 PM
moranegg updated this revision to Diff 952.Jan 9 2018, 3:10 PM
  • Amend checks with common function _get_metadata
Harbormaster completed remote builds in B1148: Diff 952.Jan 9 2018, 3:10 PM
ardumont added a comment.Jan 9 2018, 4:00 PM

A nitpick on mismatched docstrings.

swh/deposit/api/private/deposit_check.py
82

Well, the docstring no longer matches... :)

98

That would be the right location for the previous docstring.

moranegg updated this revision to Diff 953.Jan 9 2018, 4:18 PM
  • Amend checks with common function _get_metadata
Harbormaster completed remote builds in B1149: Diff 953.Jan 9 2018, 4:18 PM
moranegg marked 2 inline comments as done.Jan 9 2018, 4:19 PM
ardumont accepted this revision.Jan 9 2018, 4:21 PM
This revision is now accepted and ready to land.Jan 9 2018, 4:21 PM
Closed by commit rDDEP48eda313e9d4: Add url validation (authored by moranegg). · Explain WhyJan 9 2018, 4:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
deposit/
api/
private/
24 lines
13 lines
tests/
api/
3 lines
26 lines
11 lines

Diff 949

View Options

swh/deposit/api/private/deposit_check.py

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines def _check_archive(self, archive):
zf = zipfile.ZipFile(archive.path) zf = zipfile.ZipFile(archive.path)
zf.infolist() zf.infolist()
except Exception as e: except Exception as e:
return False return False
else: else:
return True return True
def _check_deposit_metadata(self, deposit): def _check_deposit_metadata(self, deposit):
"""Given a deposit, check each deposit request of type metadata. """Given a deposit, check each deposit request of type metadata,
ardumontUnsubmitted
Done
Inline Actions

We don't want to expose the implementation detail of how we do the check in the documentation ;)
We want to expose the intent.

A better docstring than my previous attempt would be:

Given a deposit, check its associated metadata for mandatory field presence.
   ...
ardumont: We don't want to expose the implementation detail of how we do the check in the documentation…
ardumontUnsubmitted
Done
Inline Actions

Well, the docstring no longer matches... :)

ardumont: Well, the docstring no longer matches... :)
by aggregating all metadata requests one bundle.
Args: Args:
The deposit to check metadata for. The deposit to check metadata for.
Returns: Returns:
True if the deposit's associated metadata are ok, False otherwise. True if the deposit's associated metadata are ok, False otherwise.
""" """
metadata = {} metadata = {}
for dr in self._deposit_requests(deposit, request_type=METADATA_TYPE): for dr in self._deposit_requests(deposit, request_type=METADATA_TYPE):
metadata.update(dr.metadata) metadata.update(dr.metadata)
return self._check_metadata(metadata) return self._check_metadata(metadata)
def _check_metadata(self, metadata): def _check_metadata(self, metadata):
"""Check to execute on all metadata. """Check to execute on all metadata and keeps metadata_url for url validation.
ardumontUnsubmitted
Done
Inline Actions

That would be the right location for the previous docstring.

ardumont: That would be the right location for the previous docstring.
Args: Args:
metadata (): Metadata to actually check metadata (): Metadata to actually check
ardumontUnsubmitted
Done
Inline Actions

I forgot (dict) instead of (), can you please add it?

ardumont: I forgot `(dict)` instead of `()`, can you please add it?
Returns: Returns:
True if metadata is ok, False otherwise. True if metadata is ok, False otherwise.
""" """
required_fields = (('url',), required_fields = (('url',),
('external_identifier',), ('external_identifier',),
('name', 'title'), ('name', 'title'),
('author',)) ('author',))
result = all(any(name in field result = all(any(name in field
for field in metadata for field in metadata
for name in possible_names) for name in possible_names)
for possible_names in required_fields) for possible_names in required_fields)
urls = []
for field in metadata:
if 'url' in field:
urls.append(metadata[field])
ardumontUnsubmitted
Done
Inline Actions

Please, see comments line 143 below

ardumont: Please, see comments line 143 below
self.metadata_url = urls
return result return result
def _check_url(self, client_url, metadata_urls):
validatation = any(client_url in url
for url in metadata_urls)
ardumontUnsubmitted
Done
Inline Actions

Simply returning the result here would be fine.

Also a docstring would be good.

As a note, there is a typo in the variable's name ;)

ardumont: Simply returning the result here would be fine. Also a docstring would be good. As a note…
return validatation
def process_get(self, req, collection_name, deposit_id): def process_get(self, req, collection_name, deposit_id):
"""Build a unique tarball from the multiple received and stream that """Build a unique tarball from the multiple received and stream that
content to the client. content to the client.
Args: Args:
req (Request): req (Request):
collection_name (str): Collection owning the deposit collection_name (str): Collection owning the deposit
deposit_id (id): Deposit concerned by the reading deposit_id (id): Deposit concerned by the reading
Returns: Returns:
Tuple status, stream of content, content-type Tuple status, stream of content, content-type
""" """
deposit = Deposit.objects.get(pk=deposit_id) deposit = Deposit.objects.get(pk=deposit_id)
client_url = deposit.client.url
ardumontUnsubmitted
Done
Inline Actions

I don't like setting variable outside of the __init__ construct, especially when we can avoid it :)
(I know we did it in other modules but for other reasons).

If we want to manipulate the metadata multiple times (as introduced here), we should have a method for it. Then the multiple check methods will manipulate those metadata. The intents are clearer that way.

So here, that would give:

  • _check_metadata stays as before (it only checks the metadata for field presence)
  • _check_deposit_metadata becomes just a metadata_get (it's in charge of aggregating metadata)
  • _check_url is almost like you propose but it's in charge of filtering the url fields from the metadata passed as parameter prior to the current proposed check.

Then no need for that intermediary variable.

ardumont: I don't like setting variable outside of the `__init__` construct, especially when we can avoid…
self.metadata_url = None # created in _check_metadata
problems = [] problems = []
# will check each deposit's associated request (both of type # will check each deposit's associated request (both of type
# archive and metadata) for errors # archive and metadata) for errors
archives_status = self._check_deposit_archives(deposit) archives_status = self._check_deposit_archives(deposit)
if not archives_status: if not archives_status:
problems.append('archive(s)') problems.append('archive(s)')
metadata_status = self._check_deposit_metadata(deposit) metadata_status = self._check_deposit_metadata(deposit)
if not metadata_status: if not metadata_status:
problems.append('metadata') problems.append('metadata')
deposit_status = archives_status and metadata_status url_status = self._check_url(client_url, self.metadata_url)
if not url_status:
problems.append('url')
deposit_status = archives_status and metadata_status and url_status
# if any problems arose, the deposit is rejected # if any problems arose, the deposit is rejected
if not deposit_status: if not deposit_status:
deposit.status = DEPOSIT_STATUS_REJECTED deposit.status = DEPOSIT_STATUS_REJECTED
else: else:
deposit.status = DEPOSIT_STATUS_READY deposit.status = DEPOSIT_STATUS_READY
deposit.save() deposit.save()
return (status.HTTP_200_OK, return (status.HTTP_200_OK,
json.dumps({ json.dumps({
'status': deposit.status, 'status': deposit.status,
'details': 'Some %s failed the checks.' % ( 'details': 'Some %s failed the checks.' % (
' and '.join(problems), ), ' and '.join(problems), ),
}), }),
'application/json') 'application/json')
View Options

swh/deposit/api/private/deposit_read.py

Show First 20 Lines • Show All 145 Lines • ▼ Show 20 Lines def _aggregate_metadata(self, deposit, metadata_requests):
""" """
metadata = {} metadata = {}
for req in metadata_requests: for req in metadata_requests:
metadata.update(req.metadata) metadata.update(req.metadata)
return metadata return metadata
def _retrieve_url(self, deposit, metadata):
client_url = deposit.client.url
for field in metadata:
if 'url' in field:
if client_url in metadata[field]:
return metadata[field]
def aggregate(self, deposit, requests): def aggregate(self, deposit, requests):
"""Aggregate multiple data on deposit into one unified data dictionary. """Aggregate multiple data on deposit into one unified data dictionary.
Args: Args:
deposit (Deposit): Deposit concerned by the data aggregation. deposit (Deposit): Deposit concerned by the data aggregation.
requests ([DepositRequest]): List of associated requests which requests ([DepositRequest]): List of associated requests which
need aggregation. need aggregation.
Returns: Returns:
Dictionary of data representing the deposit to inject in swh. Dictionary of data representing the deposit to inject in swh.
""" """
data = {} data = {}
# Retrieve tarballs/metadata information # Retrieve tarballs/metadata information
metadata = self._aggregate_metadata(deposit, requests) metadata = self._aggregate_metadata(deposit, requests)
# create origin_url from metadata only after deposit_check validates it
origin_url = self._retrieve_url(deposit, metadata)
# Read information metadata # Read information metadata
data['origin'] = { data['origin'] = {
'type': 'deposit', 'type': 'deposit',
'url': os.path.join(deposit.client.url.rstrip('/'), 'url': origin_url
deposit.external_id),
} }
# revision # revision
fullname = deposit.client.get_full_name() fullname = deposit.client.get_full_name()
author_committer = SWH_PERSON author_committer = SWH_PERSON
# metadata provider # metadata provider
▲ Show 20 Lines • Show All 45 Lines • Show Last 20 Lines
View Options

swh/deposit/tests/api/test_deposit_check.py

Show First 20 Lines • Show All 66 Lines • ▼ Show 20 Lines def deposit_ko(self):
args=[self.collection.name, deposit.id]) args=[self.collection.name, deposit.id])
response = self.client.get(url) response = self.client.get(url)
self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.status_code, status.HTTP_200_OK)
data = json.loads(response.content.decode('utf-8')) data = json.loads(response.content.decode('utf-8'))
self.assertEqual(data['status'], DEPOSIT_STATUS_REJECTED) self.assertEqual(data['status'], DEPOSIT_STATUS_REJECTED)
self.assertEqual(data['details'], self.assertEqual(data['details'],
'Some archive(s) and metadata failed the checks.') 'Some archive(s) and metadata and url ' +
'failed the checks.')
deposit = Deposit.objects.get(pk=deposit.id) deposit = Deposit.objects.get(pk=deposit.id)
self.assertEquals(deposit.status, DEPOSIT_STATUS_REJECTED) self.assertEquals(deposit.status, DEPOSIT_STATUS_REJECTED)
@istest @istest
def check_deposit_metadata_ok(self): def check_deposit_metadata_ok(self):
"""Proper deposit should succeed the checks (-> status ready) """Proper deposit should succeed the checks (-> status ready)
with all **MUST** metadata with all **MUST** metadata
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines
View Options

swh/deposit/tests/api/test_deposit_read_metadata.py

Show All 39 Lines def read_metadata(self):
self.assertEqual(response.status_code, self.assertEqual(response.status_code,
status.HTTP_200_OK) status.HTTP_200_OK)
self.assertEquals(response._headers['content-type'][1], self.assertEquals(response._headers['content-type'][1],
'application/json') 'application/json')
data = json.loads(response.content.decode('utf-8')) data = json.loads(response.content.decode('utf-8'))
expected_meta = { expected_meta = {
'origin': { 'origin': {
'url': 'https://hal.test.fr/some-external-id', 'url': 'https://hal-test.archives-ouvertes.fr/' +
'some-external-id',
'type': 'deposit' 'type': 'deposit'
}, },
'origin_metadata': { 'origin_metadata': {
'metadata': { 'metadata': {
'{http://www.w3.org/2005/Atom}external_identifier': '{http://www.w3.org/2005/Atom}external_identifier':
'some-external-id',
'{http://www.w3.org/2005/Atom}url':
'https://hal-test.archives-ouvertes.fr/' +
'some-external-id' 'some-external-id'
}, },
'provider': { 'provider': {
'provider_name': '', 'provider_name': '',
'provider_type': 'deposit_client', 'provider_type': 'deposit_client',
'provider_url': 'https://hal.test.fr/', 'provider_url': 'https://hal-test.archives-ouvertes.fr/',
'metadata': {} 'metadata': {}
}, },
'tool': { 'tool': {
'tool_name': 'swh-deposit', 'tool_name': 'swh-deposit',
'tool_version': '0.0.1', 'tool_version': '0.0.1',
'tool_configuration': { 'tool_configuration': {
'sword_version': '2' 'sword_version': '2'
} }
} }
}, },
'revision': { 'revision': {
'synthetic': True, 'synthetic': True,
'committer_date': None, 'committer_date': None,
'message': ': Deposit %s in collection hal' % deposit_id, 'message': ': Deposit %s in collection hal' % deposit_id,
'author': SWH_PERSON, 'author': SWH_PERSON,
'committer': SWH_PERSON, 'committer': SWH_PERSON,
'date': None, 'date': None,
'metadata': { 'metadata': {
'{http://www.w3.org/2005/Atom}external_identifier': '{http://www.w3.org/2005/Atom}external_identifier':
'some-external-id',
'{http://www.w3.org/2005/Atom}url':
'https://hal-test.archives-ouvertes.fr/' +
'some-external-id' 'some-external-id'
}, },
'type': 'tar' 'type': 'tar'
}, },
'occurrence': { 'occurrence': {
'branch': 'master' 'branch': 'master'
} }
} }
Show All 33 Lines def read_metadata_revision_with_parent(self):
self.assertEqual(response.status_code, self.assertEqual(response.status_code,
status.HTTP_200_OK) status.HTTP_200_OK)
self.assertEquals(response._headers['content-type'][1], self.assertEquals(response._headers['content-type'][1],
'application/json') 'application/json')
data = json.loads(response.content.decode('utf-8')) data = json.loads(response.content.decode('utf-8'))
expected_meta = { expected_meta = {
'origin': { 'origin': {
'url': 'https://hal.test.fr/some-external-id', 'url': 'https://hal-test.archives-ouvertes.fr/' +
'some-external-id',
'type': 'deposit' 'type': 'deposit'
}, },
'origin_metadata': { 'origin_metadata': {
'metadata': { 'metadata': {
'{http://www.w3.org/2005/Atom}external_identifier': '{http://www.w3.org/2005/Atom}external_identifier':
'some-external-id',
'{http://www.w3.org/2005/Atom}url':
'https://hal-test.archives-ouvertes.fr/' +
'some-external-id' 'some-external-id'
}, },
'provider': { 'provider': {
'provider_name': '', 'provider_name': '',
'provider_type': 'deposit_client', 'provider_type': 'deposit_client',
'provider_url': 'https://hal.test.fr/', 'provider_url': 'https://hal-test.archives-ouvertes.fr/',
'metadata': {} 'metadata': {}
}, },
'tool': { 'tool': {
'tool_name': 'swh-deposit', 'tool_name': 'swh-deposit',
'tool_version': '0.0.1', 'tool_version': '0.0.1',
'tool_configuration': { 'tool_configuration': {
'sword_version': '2' 'sword_version': '2'
} }
} }
}, },
'revision': { 'revision': {
'synthetic': True, 'synthetic': True,
'date': None, 'date': None,
'committer_date': None, 'committer_date': None,
'author': SWH_PERSON, 'author': SWH_PERSON,
'committer': SWH_PERSON, 'committer': SWH_PERSON,
'type': 'tar', 'type': 'tar',
'message': ': Deposit %s in collection hal' % deposit_id, 'message': ': Deposit %s in collection hal' % deposit_id,
'metadata': { 'metadata': {
'{http://www.w3.org/2005/Atom}external_identifier': '{http://www.w3.org/2005/Atom}external_identifier':
'some-external-id',
'{http://www.w3.org/2005/Atom}url':
'https://hal-test.archives-ouvertes.fr/' +
'some-external-id' 'some-external-id'
}, },
'parents': [swh_id] 'parents': [swh_id]
}, },
'occurrence': { 'occurrence': {
'branch': 'master' 'branch': 'master'
} }
} }
Show All 32 Lines
View Options

swh/deposit/tests/common.py

Loading...
About · Developer information · Legal