Differential D6396 Diff 24363 swh/loader/package/maven/loader.py

Changeset View

Standalone View

View Options

swh/loader/package/maven/loader.py

This file was added.

# See the AUTHORS file at the top-level directory of this distribution

# License: GNU General Public License version 3, or any later version

# See top-level LICENSE file for more information

from datetime import datetime, timezone

import hashlib

import json

import logging

from os import path

import string

from typing import Any, Dict, Iterator, Mapping, Optional, OrderedDict, Sequence, Tuple

from urllib.parse import urlparse

import attr

import iso8601

import requests

from swh.loader.package.loader import (

BasePackageInfo,

PackageLoader,

PartialExtID,

RawExtrinsicMetadataCore,

)

from swh.loader.package.utils import EMPTY_AUTHOR, release_name

from swh.model.model import (

MetadataAuthority,

MetadataAuthorityType,

ObjectType,

Release,

Sha1Git,

TimestampWithTimezone,

)

from swh.storage.interface import StorageInterface

logger = logging.getLogger(__name__)

@attr.s

class MavenPackageInfo(BasePackageInfo):

ardumontUnsubmitted

Done

According to your last version, this is now a iso8601 string.
You could even make that a datetime now providing you adapt the from_metadata call to actually parse the date string as a datetime.
And then slightly adapt the loader to "just use" that time (instead of parsing it)

ardumont: According to your last version, this is now a iso8601 string. You could even make that a…

borisbaldassariAuthorUnsubmitted

Done

Yes, now it's a string. Updated as suggested to use datetime.

borisbaldassari: Yes, now it's a string. Updated as suggested to use datetime.

time = attr.ib(type=datetime)

"""Timestamp of the last update of jar file on the server."""

vlorentzUnsubmitted

Done

time = attr.ib(type=datetime)

- """Timestamp of the last update of jar file on the server.

- Represented as an ISO8601 date string."""

+ """Timestamp of the last update of jar file on the server."""

gid = attr.ib(type=str)

It's not a string anymore

vlorentz: It's not a string anymore

gid = attr.ib(type=str)

"""Group ID of the maven artifact"""

aid = attr.ib(type=str)

"""Artifact ID of the maven artifact"""

vlorentzUnsubmitted

Done

Could you use structured data (eg. int or preferably datetime) instead of a string?
If it really needs to be a string, then please document its format (stringified unix timestamp vs stringified DOS/ZIP timestamp vs ISO8601 vs ...).

And the documentation in the constructor below should be updated to match, either way

vlorentz: Could you use structured data (eg. int or preferably datetime) instead of a string? If it…

borisbaldassariAuthorUnsubmitted

Done

I think I can make it an int.
Will try and update the doc as requested on Thursday.

borisbaldassari: I think I can make it an int. Will try and update the doc as requested on Thursday.

version = attr.ib(type=str)

"""Version of the maven artifact"""

# default format for maven artifacts

MANIFEST_FORMAT = string.Template("$gid $aid $version $url $time")

def extid(self, manifest_format: Optional[string.Template] = None) -> PartialExtID:

"""Returns a unique intrinsic identifier of this package info

vlorentzUnsubmitted

Done

It doesn't look like it can ever be a datetime.

vlorentz: It doesn't look like it can ever be a datetime.

``manifest_format`` allows overriding the class' default MANIFEST_FORMAT"""

vlorentzUnsubmitted

Done

When

vlorentz: When

borisbaldassariAuthorUnsubmitted

Done

As per the Lucene docs, it is the last update time of the file. Fixed.

borisbaldassari: As per the Lucene docs, it is the last update time of the file. Fixed.

manifest_format = manifest_format or self.MANIFEST_FORMAT

vlorentzUnsubmitted

Done

this doesn't seem to be used

vlorentz: this doesn't seem to be used

borisbaldassariAuthorUnsubmitted

Done

Interesting. It was initially copied from deposit (which i used as a template to start with). Removed it.

borisbaldassari: Interesting. It was initially copied from deposit (which i used as a template to start with).

manifest = manifest_format.substitute(

{

"gid": self.gid,

"aid": self.aid,

"version": self.version,

"url": self.url,

"time": str(self.time),

}

)

return ("maven-jar", hashlib.sha256(manifest.encode()).digest())

@classmethod

def from_metadata(cls, a_metadata: Dict[str, Any]) -> "MavenPackageInfo":

url = a_metadata["url"]

filename = a_metadata.get("filename")

time = iso8601.parse_date(a_metadata["time"])

time = time.astimezone(tz=timezone.utc)

gid = a_metadata["gid"]

aid = a_metadata["aid"]

version = a_metadata["version"]

# Rebuild POM URL.

pom_url = path.dirname(url)

vlorentzUnsubmitted

Done

Can you pass this URL from the lister instead of re-computing it here? Or are we sure this URL will always be right?

vlorentz: Can you pass this URL from the lister instead of re-computing it here? Or are we sure this URL…

borisbaldassariAuthorUnsubmitted

Done

No, we don't have it at the lister level either. And we are not sure the URL exists.

This is what I had written in another comment -- but I have the feeling that some of my answers are lost, which does not help.

We have no reliable way to know the POM; this is a heuristic that should work most of the time.
That was my point about having the reduced set of json info (gid, aid, version), which is always available, compared to the more rich but less reliable pom content.
(see comment on line 84).

borisbaldassari: No, we don't have it at the lister level either. And we are not sure the URL exists. This is…

pom_url = f"{pom_url}/{aid}-{version}.pom"

ardumontUnsubmitted

Done

@vlorentz Do we allow such side-effects [1] to occur this early in the package loading process?

I guess it's fine since it's called at the get_package_info time...
Just double checking with you.

[1] any form of interactions with the outside world (db, remote service, file, whatever can raise any form of issues...)

ardumont: @vlorentz Do we allow such side-effects [1] to occur this early in the package loading process?

vlorentzUnsubmitted

Done

It's not too early; but this is outside any exception handling, which means a crash here (and they *will* happen since it's a network request) won't update the visit status, so it must be done somewhere else, eg. by specializing build_extrinsic_directory_metadata.

vlorentz: It's not too early; but this is outside any exception handling, which means a crash here (and…

r = requests.get(pom_url, allow_redirects=True)

if r.status_code == 200:

metadata_pom = r.content

ardumontUnsubmitted

Done

url=url,

- filename=filename if filename else path.split(url)[-1],

+ filename=filename or path.split(url)[-1],

time=a_metadata["time"],

ardumont:

borisbaldassariAuthorUnsubmitted

Done

neat!

borisbaldassari: neat!

else:

vlorentzUnsubmitted

Done

are there any packages without a POM?

vlorentz: are there any packages without a POM?

borisbaldassariAuthorUnsubmitted

Done

As stated above, yes. This is what I explained in another, now invisible answer. Let me look for it.

Ah, actually it has simply moved, it's right below:
https://forge.softwareheritage.org/D6396?vs=on&id=24339#anchor-inline-47926

So we can choose between having the XML pom (complete, but not reliable) or having the JSON (incomplete, but always provided).
So it's really up to you.

borisbaldassari: As stated above, yes. This is what I explained in another, now invisible answer. Let me look…

metadata_pom = b""

return cls(

url=url,

filename=filename or path.split(url)[-1],

time=time,

gid=gid,

aid=aid,

version=version,

directory_extrinsic_metadata=[

RawExtrinsicMetadataCore(format="maven-pom", metadata=metadata_pom,),

ardumontUnsubmitted

Done

class MavenLoader(PackageLoader[MavenPackageInfo]):

- """Load jar origin's artifact files into swh archive

+ """Load source code jar origin's artifact files into swh archive

"""

ardumont:

RawExtrinsicMetadataCore(

format="maven-json", metadata=json.dumps(a_metadata).encode(),

ardumontUnsubmitted

Done

*thumbs up*

ardumont: *thumbs up*

)

class MavenLoader(PackageLoader[MavenPackageInfo]):

vlorentzUnsubmitted

Done

Could you write all the metadata here? ideally the original POM file, instead of converting to JSON

vlorentz: Could you write all the metadata here? ideally the original POM file, instead of converting to…

borisbaldassariAuthorUnsubmitted

Done

We don't have the original POM file around when loading the jar, and I don't see a reliable way to get it.

We might use the (gId, aId, version) coordinates to try to download a pom and set it as metadata, but I'm not sure how often it would work.
It depends if you'd prefer consistency and reliability (always have the above information) or completeness (not always the same information/format, but more complete when it's there).

What do you prefer?

I've set the a_metadata full info instead of the extract for now.

borisbaldassari: We don't have the original POM file around when loading the jar, and I don't see a reliable way…

"""Load source code jar origin's artifact files into swh archive

"""

visit_type = "maven"

def __init__(

self,

storage: StorageInterface,

url: str,

artifacts: Sequence[Dict[str, Any]],

extid_manifest_format: Optional[str] = None,

max_content_size: Optional[int] = None,

ardumontUnsubmitted

Done

is this still true with the latest version of the lister?

ardumont: is this still true with the latest version of the lister?

borisbaldassariAuthorUnsubmitted

Done

See next comment, yes.

We introduced last_update as a datetime, but the time parameter passed to the loader is an int. And it stil has the same meaning, i.e. last modification time on the server (as defined in the lucene index at least).

Do you think it's not?

borisbaldassari: See next comment, yes. We introduced last_update as a datetime, but the time parameter passed…

f"""Loader constructor.

For now, this is the lister's task output.

There is one, and only one, artefact (jar or zip) per version, as guaranteed by

the Maven coordinates system.

ardumontUnsubmitted

Done

is this still true (a timestamp?) regarding the latest version of the lister?

ardumont: is this still true (a timestamp?) regarding the latest version of the lister?

borisbaldassariAuthorUnsubmitted

Done

Yes, it still is a timestamp. We changed the type to int, but the content is basically the same.

borisbaldassari: Yes, it still is a timestamp. We changed the type to int, but the content is basically the same.

ardumontUnsubmitted

Done

ping?

ardumont: ping?

Args:

url: Origin url

artifacts: List of single artifact information with keys:

- **time**: the time of the last update of jar file on the server

as an iso8601 date string

- **url**: the artifact url to retrieve filename

- **filename**: optionally, the file's name

- **gid**: artifact's groupId

- **aid**: artifact's artifactId

- **version**: artifact's version

extid_manifest_format: template string used to format a manifest,

which is hashed to get the extid of a package.

Defaults to {MavenPackageInfo.MANIFEST_FORMAT!r}

"""

ardumontUnsubmitted

Done

else string.Template(extid_manifest_format)

)

self.snapshot_append = snapshot_append

+ self.version_artifact: Dict[str, Dict] = {jar.get(version): jar for jar in artifacts if jar.get(version)}

def get_versions(self) -> Sequence[str]:

Internal state of version to the associated jar (assuming on jar per version).
(you may want to use an OrderedDict to keep the order on the versions though due to the current default version implementation).

ardumont: Internal state of version to the associated jar (assuming on jar per version). (you may want to…

borisbaldassariAuthorUnsubmitted

Done

Internal state of version to the associated jar (assuming on jar per version).

Assumption is correct.
Added the proposed line (adding quotes around "version") for readability and completeness though.

(you may want to use an OrderedDict to keep the order on the versions though due to the current default version implementation).

Done.

borisbaldassari: > Internal state of version to the associated jar (assuming on jar per version). Assumption is…

super().__init__(storage=storage, url=url, max_content_size=max_content_size)

self.artifacts = artifacts # assume order is enforced in the lister

self.version_artifact: OrderedDict[str, Dict[str, Any]]

self.version_artifact = OrderedDict(

{str(jar["version"]): jar for jar in artifacts if jar["version"]}

)

vlorentzUnsubmitted

Done

What is this for?

vlorentz: What is this for?

borisbaldassariAuthorUnsubmitted

Done

Hum. Interesting, thanks for pointing that out.

I thought it was somehow used by the loading mechanism to register its state, but on a second look it's probably just a carryover from the example I took in the archive loader. Removed.

borisbaldassari: Hum. Interesting, thanks for pointing that out. I thought it was somehow used by the loading…

ardumontUnsubmitted

Done

def get_versions(self) -> Sequence[str]:

- versions = []

- for jar in self.artifacts:

- v = jar.get("version")

- if v:

- versions.append(v)

- return versions

+ return list(self.version_artifact.keys())

def get_default_version(self) -> str:

to stay consistent with the other suggestion.

ardumont: to stay consistent with the other suggestion.

def get_versions(self) -> Sequence[str]:

return list(self.version_artifact.keys())

ardumontUnsubmitted

Done

def get_default_version(self) -> str:

- # Returning the last item -- there should be only one version anyway.

- return self.artifacts[-1]["version"]

+ # Default version is the last item return self.artifacts[-1]["version"]

Assuming my early assumption about 1 artifact per version is correct, mention it before in the constructor maybe.

ardumont: Assuming my early assumption about 1 artifact per version is correct, mention it before in the…

def get_default_version(self) -> str:

# Default version is the last item

return self.artifacts[-1]["version"]

def get_metadata_authority(self):

vlorentzUnsubmitted

Done

You can remove this, it's not used

vlorentz: You can remove this, it's not used

p_url = urlparse(self.url)

return MetadataAuthority(

type=MetadataAuthorityType.FORGE,

url=f"{p_url.scheme}://{p_url.netloc}/",

metadata={},

vlorentzUnsubmitted

Done

Is the version ever missing from the input? Can we be sure there is at most one package with a missing version per (gid, aid)?

vlorentz: Is the version ever missing from the input? Can we be sure there is at most one package with a…

borisbaldassariAuthorUnsubmitted

Done

No, the lister is expected to always provide the version (alongside the gid and aid). If you're wondering why having this loop, it's because it's more elegant than artefact[0] and it feels more safe. But I can change it.

(Note that there is only one artefact in the artifact list, as identified by the url.)

borisbaldassari: No, the lister is expected to //always// provide the version (alongside the gid and aid). If…

vlorentzUnsubmitted

Done

I don't think this is addressed, there is still this code:

self.version_artifact = OrderedDict(
    {str(jar.get("version")): jar for jar in artifacts if jar.get("version")}
)

vlorentz: I don't think this is addressed, there is still this code: ``` self.version_artifact =…

borisbaldassariAuthorUnsubmitted

Done

I don't know. I'm not sure to understand what you mean or expect.

What I know:

This loader is executed for a single jar/zip, or in other words for a single artefact, defined by the URL. There is no case where there would be more than one artefact involved.
The gid, aid, version are always provided by the lister. Without this information, the jar loader is nothing more than an archive loader.

To answer your initial comment, when the loader is invoked there is indeed at most one package (whatever the gid, aid, version).

Is there a specific case/situation you have in mind?

borisbaldassari: I don't know. I'm not sure to understand what you mean or expect. What I know: * This loader…

vlorentzUnsubmitted

Done

ping?

vlorentz: ping?

)

def get_package_info(self, version: str) -> Iterator[Tuple[str, MavenPackageInfo]]:

a_metadata = self.version_artifact[version]

yield release_name(a_metadata["version"]), MavenPackageInfo.from_metadata(

ardumontUnsubmitted

Done

metadata={},

)

def get_package_info(self, version: str) -> Iterator[Tuple[str, MavenPackageInfo]]:

- a_metadata = self.artifacts[0]

+ a_metadata = self.version_artifact[version]

yield release_name(a_metadata["version"]), MavenPackageInfo.from_metadata(

a_metadata

)

def build_revision(

That smells funny to me. You will always load the metadata from the first artifact here.
You should retrieve the artifact corresponding to the version here as proposed.

You need to have an inverted dict from version to artifact initialized before this call.
(i've amended some other part of the diff to consistently propose that ;)

ardumont: That smells funny to me. You will always load the metadata from the first artifact here. You…

borisbaldassariAuthorUnsubmitted

Done

Thanks for the whole proposed change. Yes, I agree it's much more clean this way.

borisbaldassari: Thanks for the whole proposed change. Yes, I agree it's much more clean this way.

ardumontUnsubmitted

Done

push that next to the parsing step?
that may even need to be part of the lister?

ardumont: push that next to the parsing step? that may even need to be part of the lister?

borisbaldassariAuthorUnsubmitted

Done

Moved that next to the parsing step.

I don't think it should be put in the lister, as what is yielded by the lister is already a UTC time. This line simply sets the loader local var as a UTC time to be interpreted correctly.

borisbaldassari: Moved that next to the parsing step. I don't think it should be put in the lister, as what is…

a_metadata

)

vlorentzUnsubmitted

Done

dead code according to the type of p_info.time

vlorentz: dead code according to the type of `p_info.time`

def build_release(

self, p_info: MavenPackageInfo, uncompressed_path: str, directory: Sha1Git

) -> Optional[Release]:

msg = f"Synthetic release for archive at {p_info.url}\n".encode("utf-8")

ardumontUnsubmitted

Done

I believe that it is now the else conditional which is the dead code [1]

[1] regarding the latest version of the lister (which does the timestamp conversion and parsing).

ardumont: I believe that it is now the else conditional which is the dead code [1] [1] regarding the…

borisbaldassariAuthorUnsubmitted

Done

Hum. No, I don't think so. The last_update parameter has been added as a datetime for the scheduler, but the time parameter passed to the loader is still an int.

Am I missing something?

borisbaldassari: Hum. No, I don't think so. The last_update parameter has been added as a datetime for the…

# time is an iso8601 date

normalized_time = TimestampWithTimezone.from_datetime(p_info.time)

return Release(

name=p_info.version.encode(),

message=msg,

date=normalized_time,

author=EMPTY_AUTHOR,

target=directory,

target_type=ObjectType.DIRECTORY,

synthetic=True,

)

def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:

last_snapshot = self.last_snapshot()

return last_snapshot.to_dict()["branches"] if last_snapshot else {}

vlorentzUnsubmitted

Done

else: # assume it's a timestamp (in milliseconds)

- raw_time = int(str(p_info.time))

+ raw_time = int(time)

parsed_time = datetime.fromtimestamp(raw_time / 1e3)

vlorentz:

vlorentzUnsubmitted

Done

This should be the empty Person, as documented

vlorentz: This should be the empty Person, as documented

borisbaldassariAuthorUnsubmitted

Done

I've updated the documentation to use "SWH robot", for consistency with the deposit and archive loaders (which I've used intensively as examples).

borisbaldassari: I've updated the documentation to use "SWH robot", for consistency with the deposit and archive…

vlorentzUnsubmitted

Done

ping?

vlorentz: ping?

borisbaldassariAuthorUnsubmitted

Done

As mentioned I've updated the package-loader-specifications.rst file to be consistent with other loaders. It was a mistake from me to create the maven entry with an empty person at first.
Is there anything else I should do?

borisbaldassari: As mentioned I've updated the package-loader-specifications.rst file to be consistent with…