Merge branch 'master' into pytest

.github/workflows/continuous_integration.yml

@@ -21,7 +21,7 @@ jobs:
     runs-on: ubuntu-20.04
     strategy:
       matrix:
-        qgis_version: [release-3_16, latest]
+        qgis_version: [release-3_16, release-3_22, release-3_28, latest]
     env:
       QGIS_TEST_VERSION: ${{ matrix.qgis_version }}
     steps:

.mypy.ini

@@ -0,0 +1,2 @@
+[mypy]
+disable_error_code = var-annotated

.pre-commit-config.yaml

@@ -38,3 +38,9 @@ repos:
     rev: "3.9.0"
     hooks:
       - id: flake8
+
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    rev: 'v1.3.0'
+    hooks:
+      - id: mypy
+        additional_dependencies: []

layer.py

@@ -3,7 +3,7 @@
 import re
 import shutil
 from enum import Enum
-from typing import Dict, Optional
+from typing import Dict, List, Optional
 
 from qgis.core import (
     QgsAttributeEditorField,
@@ -20,7 +20,18 @@
 from qgis.PyQt.QtCore import QCoreApplication
 from qgis.PyQt.QtXml import QDomDocument
 
+from .utils.bad_layer_handler import bad_layer_handler
 from .utils.file_utils import slugify
+from .utils.logger import logger
+
+
+class ExpectedVectorLayerError(Exception):
+    ...
+
+
+class UnsupportedPrimaryKeyError(Exception):
+    ...
+
 
 # When copying files, if any of the extension in any of the groups is found,
 # other files with the same extension in the same folder will be copied as well.
@@ -98,6 +109,16 @@ class AttachmentType(
         AUDIO = 3  # QgsExternalResourceWidget.Audio
         VIDEO = 4  # QgsExternalResourceWidget.Video
 
+    class PackagePreventionReason(Enum):
+        INVALID = 1
+        UNSUPPORTED_DATASOURCE = 2
+        LOCALIZED_PATH = 3
+
+    REASONS_TO_REMOVE_LAYER = (
+        PackagePreventionReason.INVALID,
+        PackagePreventionReason.UNSUPPORTED_DATASOURCE,
+    )
+
     ATTACHMENT_EXPRESSIONS = {
         AttachmentType.FILE: "'files/{layername}_' || format_date(now(),'yyyyMMddhhmmsszzz') || '_{{filename}}'",
         AttachmentType.IMAGE: "'DCIM/{layername}_' || format_date(now(),'yyyyMMddhhmmsszzz') || '.{{extension}}'",
@@ -203,15 +224,17 @@ def cloud_action(self):
     def cloud_action(self, action):
         self._cloud_action = action
 
-    def get_attachment_field_type(self, field_name: str) -> None:
+    def get_attachment_field_type(self, field_name: str) -> Optional[AttachmentType]:
         if self.layer.type() != QgsMapLayer.VectorLayer:
-            return None
+            raise ExpectedVectorLayerError(
+                f'Cannot get attachment field types for non-vector layer "{self.layer.name()}"!'
+            )
 
         field_idx = self.layer.fields().indexFromName(field_name)
         ews = self.layer.editorWidgetSetup(field_idx)
 
         if ews.type() != "ExternalResource":
-            return
+            return None
 
         resource_type = (
             ews.config()["DocumentViewer"] if "DocumentViewer" in ews.config() else 0
@@ -241,6 +264,7 @@ def get_attachment_type_by_int_value(self, value: int) -> AttachmentType:
 
     def attachment_naming(self, field_name) -> str:
         attachment_type = self.get_attachment_field_type(field_name)
+        assert attachment_type is not None
         default_name_setting_value = self.ATTACHMENT_EXPRESSIONS[
             attachment_type
         ].format(layername=slugify(self.layer.name()))
@@ -489,11 +513,91 @@ def filename(self) -> str:
 
     @property
     def is_localized_path(self) -> bool:
+        # on QFieldCloud localized layers will be invalid and therefore we get the layer source from `bad_layer_handler`
+        source = bad_layer_handler.invalid_layer_sources_by_id.get(self.layer.id())
+        if source:
+            return source.startswith("localized:")
+
         path_resolver = self.project.pathResolver()
         path = path_resolver.writePath(self.filename)
 
         return path.startswith("localized:")
 
+    @property
+    def package_prevention_reasons(
+        self,
+    ) -> List["LayerSource.PackagePreventionReason"]:
+        reasons = []
+
+        # remove unsupported layers from the packaged project
+        if not self.is_supported:
+            reasons.append(LayerSource.PackagePreventionReason.UNSUPPORTED_DATASOURCE)
+
+        # do not package the layers within localized paths (stored outside project dir and shared among multiple projects)
+        if self.is_localized_path:
+            reasons.append(LayerSource.PackagePreventionReason.LOCALIZED_PATH)
+        # remove invalid layers from the packaged project
+        # NOTE localized layers will be always invalid on QFieldCloud
+        elif not self.layer.isValid():
+            reasons.append(LayerSource.PackagePreventionReason.INVALID)
+
+        return reasons
+
+    @property
+    def pk_attr_name(self) -> str:
+        try:
+            return self.get_pk_attr_name()
+        except (ExpectedVectorLayerError, UnsupportedPrimaryKeyError):
+            return ""
+
+    def get_pk_attr_name(self) -> str:
+        pk_attr_name: str = ""
+
+        if self.layer.type() != QgsMapLayer.VectorLayer:
+            raise ExpectedVectorLayerError()
+
+        pk_indexes = self.layer.primaryKeyAttributes()
+        fields = self.layer.fields()
+
+        if len(pk_indexes) == 1:
+            pk_attr_name = fields[pk_indexes[0]].name()
+        elif len(pk_indexes) > 1:
+            raise UnsupportedPrimaryKeyError(
+                "Composite (multi-column) primary keys are not supported!"
+            )
+        else:
+            logger.info(
+                f'Layer "{self.layer.name()}" does not have a primary key. Trying to fallback to `fid`…'
+            )
+
+            # NOTE `QgsFields.lookupField(str)` is case insensitive (so we support "fid", "FID", "Fid" etc),
+            # but also looks for the field alias, that's why we check the `field.name().lower() == "fid"`
+            fid_idx = fields.lookupField("fid")
+            if fid_idx >= 0 and fields.at(fid_idx).name().lower() == "fid":
+                fid_name = fields.at(fid_idx).name()
+                logger.info(
+                    f'Layer "{self.layer.name()}" does not have a primary key so it uses the `fid` attribute as a fallback primary key. '
+                    "This is an unstable feature! "
+                    "Consider [converting to GeoPackages instead](https://docs.qfield.org/get-started/tutorials/get-started-qfc/#configure-your-project-layers-for-qfield). "
+                )
+                pk_attr_name = fid_name
+
+        if not pk_attr_name:
+            raise UnsupportedPrimaryKeyError(
+                f'Layer "{self.layer.name()}" neither has a primary key, nor an attribute `fid`! '
+            )
+
+        if "," in pk_attr_name:
+            raise UnsupportedPrimaryKeyError(
+                'Comma in field name "{pk_attribute_name}" is not allowed!'
+            )
+
+        logger.info(
+            f'Layer "{self.layer.name()}" has attribute "{pk_attr_name}" as a primary key.'
+        )
+
+        return pk_attr_name
+
     def copy(self, target_path, copied_files, keep_existent=False):
         """
         Copy a layer to a new path and adjust its datasource.
@@ -593,7 +697,7 @@ def convert_to_gpkg(self, target_path):
 
         layer_subset_string = self.layer.subsetString()
         if new_source == "":
-            pattern = re.compile("[\W_]+")  # NOQA
+            pattern = re.compile(r"[\W_]+")  # NOQA
             cleaned_name = pattern.sub("", self.layer.name())
             dest_file = os.path.join(target_path, "{}.gpkg".format(cleaned_name))
             suffix = 0

offline_converter.py

@@ -48,7 +48,8 @@
 
 from .layer import LayerSource, SyncAction
 from .project import ProjectConfiguration, ProjectProperties
-from .utils.file_utils import copy_attachments, isascii
+from .utils.file_utils import copy_attachments
+from .utils.logger import logger
 from .utils.qgis import make_temp_qgis_file, open_project
 from .utils.xml import get_themapcanvas
 
@@ -63,7 +64,6 @@ class LayerData(TypedDict):
         source: str
         type: int
         fields: Optional[QgsFields]
-        pk_names: Optional[List[str]]
 
 else:
     LayerData = Dict
@@ -99,7 +99,6 @@ def __init__(
         self.__convertor_progress = None  # for processing feedback
         self.__layers = list()
         self.__layer_data_by_id: Dict[str, LayerData] = {}
-        self.__layer_data_by_name: Dict[str, LayerData] = {}
         self.__offline_layer_names: List[str] = []
 
         # elipsis workaround
@@ -185,79 +184,72 @@ def _convert(self, project: QgsProject) -> None:
         self.total_progress_updated.emit(0, 100, self.trUtf8("Converting project…"))
         self.__layers = list(project.mapLayers().values())
 
+        if self.create_basemap and self.project_configuration.create_base_map:
+            self._export_basemap()
+
+        copied_files = list()
+
         # We store the pks of the original vector layers
-        for layer in self.__layers:
-            pk_names = None
-            if layer.type() == QgsMapLayer.VectorLayer:
-                pk_names = []
-                for idx in layer.primaryKeyAttributes():
-                    pk_name = layer.fields()[idx].name()
-                    # and we check that the primary key fields names don't have a comma in the name
-                    if "," in pk_name:
-                        raise ValueError("Comma in field names not allowed")
-                    pk_names.append(pk_name)
-
-                layer.setCustomProperty(
-                    "QFieldSync/sourceDataPrimaryKeys", ",".join(pk_names)
-                )
+        for layer_idx, layer in enumerate(self.__layers):
+            layer_source = LayerSource(layer)
+
+            # NOTE if the layer is prevented from packaging it does NOT mean we have to remove it, but we cannot collect any layer metadata (e.g. if the layer is localized path).
+            # NOTE cache the value, since we might remove the layer and the reasons cannot be recalculated
+            package_prevention_reasons = layer_source.package_prevention_reasons
+            if package_prevention_reasons:
+                # remove the layer if it is invalid or not supported datasource on QField
+                for reason in package_prevention_reasons:
+                    if reason in LayerSource.REASONS_TO_REMOVE_LAYER:
+                        logger.warning(
+                            f'Layer "{layer.name()}" cannot be packaged and will be removed because "{reason}".'
+                        )
+                        project.removeMapLayer(layer)
+                        break
+                    else:
+                        logger.warning(
+                            f'Layer "{layer.name()}" cannot be packaged due to "{reason}", skipping…'
+                        )
+
+                # do not attempt to package the layer
+                continue
 
             layer_data: LayerData = {
                 "id": layer.id(),
                 "name": layer.name(),
                 "type": layer.type(),
                 "source": layer.source(),
                 "fields": layer.fields() if hasattr(layer, "fields") else None,
-                "pk_names": pk_names,
             }
 
-            self.__layer_data_by_id[layer.id()] = layer_data
-            self.__layer_data_by_name[layer.name()] = layer_data
-
-            layer.setCustomProperty("QFieldSync/remoteLayerId", layer.id())
-
-        if self.create_basemap and self.project_configuration.create_base_map:
-            self._export_basemap()
-
-        # Loop through all layers and copy/remove/offline them
-        copied_files = list()
-        non_ascii_filename_layers: Dict[str, str] = {}
-        non_utf8_encoding_layers: Dict[str, str] = {}
-        for layer_idx, layer in enumerate(self.__layers):
-            self.total_progress_updated.emit(
-                layer_idx - len(self.__offline_layers),
-                len(self.__layers),
-                self.trUtf8("Copying layers…"),
-            )
-
-            layer_source = LayerSource(layer)
             layer_action = (
                 layer_source.action
                 if self.export_type == ExportType.Cable
                 else layer_source.cloud_action
             )
 
-            if not layer.isValid():
-                project.removeMapLayer(layer)
-                continue
-
-            if not layer_source.is_supported:
-                project.removeMapLayer(layer)
-                continue
+            if layer.type() == QgsMapLayer.VectorLayer:
+                if layer_source.pk_attr_name:
+                    # NOTE even though `QFieldSync/sourceDataPrimaryKeys` is in plural, we never supported composite (multi-column) PKs and always stored a single value
+                    layer.setCustomProperty(
+                        "QFieldSync/sourceDataPrimaryKeys", layer_source.pk_attr_name
+                    )
+                else:
+                    # The layer has no supported PK, so we mark it as readonly and just copy it when packaging in the cloud
+                    if self.export_type == ExportType.Cloud:
+                        layer_action = SyncAction.NO_ACTION
+                        layer.setReadOnly(True)
+                        layer.setCustomProperty("QFieldSync/unsupported_source_pk", "1")
 
-            if layer_source.is_file and not isascii(layer_source.filename):
-                non_ascii_filename_layers[layer.name()] = layer_source.filename
+            self.__layer_data_by_id[layer.id()] = layer_data
 
-            if layer_source.is_localized_path:
-                continue
+            # `QFieldSync/remoteLayerId` should be equal to `remoteLayerId`, which is already set by `QgsOfflineEditing`. We add this as a copy to have control over this attribute that might suddenly change on QGIS.
+            layer.setCustomProperty("QFieldSync/remoteLayerId", layer.id())
 
-            if (
-                layer.type() == QgsMapLayer.VectorLayer
-                and layer.dataProvider()
-                and layer.dataProvider().encoding() != "UTF-8"
-                # some providers return empty string as encoding, just ignore them
-                and layer.dataProvider().encoding() != ""
-            ):
-                non_utf8_encoding_layers[layer.name()] = layer.dataProvider().encoding()
+            self.total_progress_updated.emit(
+                layer_idx - len(self.__offline_layers),
+                len(self.__layers),
+                self.trUtf8("Copying layers…"),
+            )
 
             if layer_action == SyncAction.OFFLINE:
                 if self.project_configuration.offline_copy_only_aoi:
@@ -282,30 +274,6 @@ def _convert(self, project: QgsProject) -> None:
             elif layer_action == SyncAction.REMOVE:
                 project.removeMapLayer(layer)
 
-        if non_ascii_filename_layers:
-            layers = ", ".join(
-                [
-                    f'"{name}" at "{path}"'
-                    for name, path in non_ascii_filename_layers.items()
-                ]
-            )
-            message = self.tr(
-                "Some layers are stored at file paths that are not ASCII encoded: {}. Working with paths that are not in ASCII might cause problems. It is highly recommended to rename them to ASCII encoded paths."
-            ).format(layers)
-            self.warning.emit(self.tr("QFieldSync"), message)
-
-        if non_utf8_encoding_layers:
-            layers = ", ".join(
-                [
-                    f"{name} ({encoding})"
-                    for name, encoding in non_utf8_encoding_layers.items()
-                ]
-            )
-            message = self.tr(
-                "Some layers do not use UTF-8 encoding: {}. Working with layers that do not use UTF-8 encoding might cause problems. It is highly recommended to convert them to UTF-8 encoded layers."
-            ).format(layers)
-            self.warning.emit(self.tr("QFieldSync"), message)
-
         export_project_filename = self.export_folder.joinpath(
             f"{self.original_filename.stem}_qfield.qgs"
         )
@@ -330,7 +298,7 @@ def _convert(self, project: QgsProject) -> None:
             copy_attachments(
                 self.original_filename.parent,
                 export_project_filename.parent,
-                source_dir,
+                Path(source_dir),
             )
         try:
             # Run the offline plugin for gpkg