refactor: replace resource instance factory (DEV-2876)

[BalduinLandolt]

No description provided.

[linear]

DEV-2876 remove use of resource instance factory

[BalduinLandolt]

This PR is by no means perfect; especially one should add more tests, which now should already be considerably easier. But I would like to keep the PR size manageable, so I would prefer not tho have to do this in this one.

[jnussbaum]

Overall a very cool PR, congrats, and thanks for tidying up this mess!
There are a lot of minor comments.
The only major concern are the errors raised in resource_create_client.py. As far as I see, they would be triggered during the upload. But in xmlupload.py, line 354, only BaseErrors are catched. So during a very long upload, it could happen that the program crashes because of bad input data. This could easily be prevented if we make sure that the only error that is raised in resource_create_client.py is BaseError. (Or, if xmlupload.py catches Exception.)

[jnussbaum]

Another observation: There seem to be several id2iri_mappings that aren't synchronized. E.g. in xmlupload.py, line 338, you update one of them, but the one inside resource_create_client is still outdated. This will probably cause a problem in resource_create_client.py, line 315 (see my comment here: #594 (comment))

Yet another id2iri_mapping is created on line 156 of xmlupload.py (but that doesn't seem to be problematic)

[BalduinLandolt]

Another observation: There seem to be several id2iri_mappings that aren't synchronized. E.g. in xmlupload.py, line 338, you update one of them, but the one inside resource_create_client is still outdated. This will probably cause a problem in resource_create_client.py, line 315 (see my comment here: #594 (comment))

Yet another id2iri_mapping is created on line 156 of xmlupload.py (but that doesn't seem to be problematic)

I can't see any code path where this would be an issue - from what I can tell, there is really only one instance around (apart from the one on line 156, which as you mention, isn't relevant, because it is just there to ensure initialization of the variable in an error case.
But it's very well possible that I'm missing something. (And I strongly dislike this mutable state that's potentially modified all over the place. So if you can think of a nicer way, I'm happy... writing this, I think we could turn this IRI-registry into a little service so that it's at least compartmentalised properly, what do you think?)

[jnussbaum]

Another observation: There seem to be several id2iri_mappings that aren't synchronized. E.g. in xmlupload.py, line 338, you update one of them, but the one inside resource_create_client is still outdated. This will probably cause a problem in resource_create_client.py, line 315 (see my comment here: #594 (comment))
Yet another id2iri_mapping is created on line 156 of xmlupload.py (but that doesn't seem to be problematic)

I can't see any code path where this would be an issue - from what I can tell, there is really only one instance around (apart from the one on line 156, which as you mention, isn't relevant, because it is just there to ensure initialization of the variable in an error case. But it's very well possible that I'm missing something. (And I strongly dislike this mutable state that's potentially modified all over the place. So if you can think of a nicer way, I'm happy... writing this, I think we could turn this IRI-registry into a little service so that it's at least compartmentalised properly, what do you think?)

• I don't understand how xmlupload.py, line 338 manages to update the mapping inside the resource_create_client. As far as I can see, resource_create_client has an empty mapping, which is never updated?
• A service sounds exciting. Do you mean a listener, i.e. a class instance that holds the IRI-registry as a field, and that has an update() method that is called from the various places?

[BalduinLandolt]

Another observation: There seem to be several id2iri_mappings that aren't synchronized. E.g. in xmlupload.py, line 338, you update one of them, but the one inside resource_create_client is still outdated. This will probably cause a problem in resource_create_client.py, line 315 (see my comment here: #594 (comment))
Yet another id2iri_mapping is created on line 156 of xmlupload.py (but that doesn't seem to be problematic)

I can't see any code path where this would be an issue - from what I can tell, there is really only one instance around (apart from the one on line 156, which as you mention, isn't relevant, because it is just there to ensure initialization of the variable in an error case. But it's very well possible that I'm missing something. (And I strongly dislike this mutable state that's potentially modified all over the place. So if you can think of a nicer way, I'm happy... writing this, I think we could turn this IRI-registry into a little service so that it's at least compartmentalised properly, what do you think?)

• I don't understand how xmlupload.py, line 338 manages to update the mapping inside the resource_create_client. As far as I can see, resource_create_client has an empty mapping, which is never updated?
• A service sounds exciting. Do you mean a listener, i.e. a class instance that holds the IRI-registry as a field, and that has an update() method that is called from the various places?

If you have a look at this bit of code:

dsp-tools/src/dsp_tools/utils/xmlupload/xmlupload.py

Lines 279 to 349 in 922703b

	def _upload_resources(
	resources: list[XMLResource],
	imgdir: str,
	sipi_server: Sipi,
	permissions_lookup: dict[str, Permissions],
	con: Connection,
	config: UploadConfig,
	project_client: ProjectClient,
	list_client: ListClient,
	) -> tuple[dict[str, str], list[str]]:
	"""
	Iterates through all resources and tries to upload them to DSP.
	If a temporary exception occurs, the action is repeated until success,
	and if a permanent exception occurs, the resource is skipped.
	Args:
	resources: list of XMLResources to upload to DSP
	imgdir: folder containing the multimedia files
	sipi_server: Sipi instance
	permissions_lookup: maps permission strings to Permission objects
	con: connection to DSP
	config: the upload configuration
	project_client: a client for HTTP communication with the DSP-API
	list_client: a client for HTTP communication with the DSP-API
	Returns:
	id2iri_mapping, failed_uploads
	"""
	id2iri_mapping: dict[str, str] = {}
	failed_uploads: list[str] = []
	project_iri = project_client.get_project_iri()
	json_ld_context = get_json_ld_context_for_project(project_client.get_ontology_name_dict())
	listnode_lookup = list_client.get_list_node_id_to_iri_lookup()
	resource_create_client = ResourceCreateClient(
	con=con,
	project_iri=project_iri,
	json_ld_context=json_ld_context,
	id2iri_mapping=id2iri_mapping,
	permissions_lookup=permissions_lookup,
	listnode_lookup=listnode_lookup,
	)
	for i, resource in enumerate(resources):
	bitstream_information = None
	if bitstream := resource.bitstream:
	bitstream_information = handle_bitstream(
	resource=resource,
	bitstream=bitstream,
	preprocessing_done=config.preprocessing_done,
	permissions_lookup=permissions_lookup,
	sipi_server=sipi_server,
	imgdir=imgdir,
	)
	if not bitstream_information:
	failed_uploads.append(resource.id)
	continue
	res = _create_resource(resource, bitstream_information, resource_create_client)
	if not res:
	failed_uploads.append(resource.id)
	continue
	iri, label = res
	id2iri_mapping[resource.id] = iri
	resource_designation = f"'{label}' (ID: '{resource.id}', IRI: '{iri}')"
	print(f"Created resource {i+1}/{len(resources)}: {resource_designation}")
	logger.info(f"Created resource {i+1}/{len(resources)}: {resource_designation}")
	return id2iri_mapping, failed_uploads

• L 307 creates an empty dict.
• L 318 passes the dict to the ResourceCreateClient -> the client holds on to a reference of that dict
• L 343 updates the dict repeatedly -> inside the client, the dict will update

Does this make sense?

But I will try to implement an IRI resolver to hide this ugly behaviour behind a nice facade :)