[SYNPY-1414] Project model finishing touches #1067

BryanFauble · 2024-02-13T22:58:32Z

Problem:

Not all fields were being saved on the Project
I missed the is_restricted flag at the folder level that gets passed to syn.store()
We are not able to store projects or folders by ID only in a non-destructive way. When we did an upsert action (Called createOrUpdate) we did so only after we got an HTTP 409 conflict, but only if we did not specify the ID of the resource. This led to inconsistent behavior if you supplied an ID, instead of a name when saving resources like the Project.
You could not specify project alias through the client before
copy needed to be added to project
Copy on the project/folder level could take in parameters that affect how files are copied
Annotations were not copied over during a project copy

Solution:

Adding missing fields to be saved on project
Adding an is_restricted flag to the folder level
Allow storing projects/folders by ID with an upsert by reading for the stored synapse instance if we didn't already communicate with Synapse for that instance before. Updating when an 'upsert' logic occurs to be before we attempt to persist anything to Synapse. First we do a check to find the ID of the resource being updated, and if found, retrieve the current state from Synapse. We then merge the object in Synapse to the requested changed - Only if there is an update will we then store the data via an HTTP PUT. If there are no change to be made we do not make the call. If the resource is not found in Synapse we perform a create via HTTP POST.
Adding project alias
Adding copy to Project
Adding in project/folder level parameters for file copying
Copy project annotations

Testing:

Added the POC script for working with a project object
Added unit/integration testing

pep8speaks · 2024-02-13T22:58:43Z

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file synapseclient/core/utils.py:

Line 1359:89: E501 line too long (97 > 88 characters)

In the file synapseclient/models/folder.py:

Line 125:89: E501 line too long (89 > 88 characters)
Line 153:89: E501 line too long (91 > 88 characters)
Line 387:89: E501 line too long (89 > 88 characters)
Line 389:89: E501 line too long (89 > 88 characters)
Line 392:89: E501 line too long (90 > 88 characters)

In the file synapseclient/models/project.py:

Line 188:89: E501 line too long (91 > 88 characters)
Line 233:89: E501 line too long (102 > 88 characters)
Line 376:89: E501 line too long (99 > 88 characters)
Line 442:89: E501 line too long (89 > 88 characters)
Line 444:89: E501 line too long (89 > 88 characters)
Line 447:89: E501 line too long (90 > 88 characters)
Line 456:89: E501 line too long (89 > 88 characters)
Line 463:89: E501 line too long (111 > 88 characters)

In the file tests/integration/synapseclient/models/test_project.py:

Line 555:89: E501 line too long (89 > 88 characters)

In the file tests/unit/synapseclient/models/unit_test_project.py:

Line 469:89: E501 line too long (109 > 88 characters)

Comment last updated at 2024-02-15 21:20:43 UTC

…ion copy

docs/scripts/object_orientated_programming_poc/oop_poc_project.py

synapseclient/client.py

synapseclient/models/project.py

sonarcloud · 2024-02-15T23:02:28Z

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
97.2% Coverage on New Code
8.7% Duplication on New Code

See analysis details on SonarCloud

docs/scripts/object_orientated_programming_poc/oop_poc_project.py

thomasyu888 · 2024-02-19T18:33:17Z

synapseclient/core/utils.py

+
+
+def merge_dataclass_entities(
+    source: typing.Union["Project", "Folder"],


I can see this function being helpful for any Entities that support annotations. (File, Table, Dataset, etc). Not sure if we need to add it in right here, but just a note

Agreed - Since the FileEntity is currently doing this is a special path in syn.store I did not add this there yet. I intend that we'll follow a similar pattern of all Entities.

Do you think that there is an "annotation" mixIn here where the classes that can have annotations can do something like sets in python and have .update functionality?

Can you elaborate a bit more on your thoughts on this? I see what sets is doing, but I'm unclear what you mean about applying it in this use-case.

Hmm, I didn't realize that dataclasses had implemented the __eq__ and __hash__ under the hood. I was wondering if its possible to just do something like

MyFileEntity.update(MySecondFileEntity) and it would just update the annotations.

It may be helpful to be able to do set operations on groups of entities, but I just haven't done that yet.

MyFileEntity.update(MySecondFileEntity) and it would just update the annotations.

I don't think so in this case because we are setting annotations to be a dict. This function was built out of the need that dataclasses didn't have any nifty mechanism I could find that would do a merge.

docs/scripts/object_orientated_programming_poc/oop_poc_project.py

thomasyu888

🔥 Nice work here! I'm going to pre-approve, just some minor comments.

thomasyu888 · 2024-02-19T21:12:42Z

synapseclient/models/folder.py

@@ -324,6 +370,8 @@ async def copy(
        parent_id: str,
        copy_annotations: bool = True,
        exclude_types: Optional[List[str]] = None,
+        file_update_existing: bool = False,


This makes sense in a Folder context, but you can also update existing projects, tables, and other entities.

Per this documentation here - It is explicit that updateExisting is only applicable in the case of a File copy. Agreed on the Table front - However, the boolean isn't being used during a table copy.

Shows how much I remember for a function I wrote. 🙃

thomasyu888 · 2024-02-19T21:14:29Z

synapseclient/models/project.py

@@ -113,25 +120,25 @@ class Project(AccessControllable, StorableContainer):
    concurrent updates. Since the E-Tag changes every time an entity is updated it
    is used to detect when a client's current representation of an entity is out-of-date."""

-    created_on: Optional[str] = None
-    """The date this entity was created."""
+    created_on: Optional[str] = field(default=None, compare=False)


TIL: compare=False - very neat!

thomasyu888 · 2024-02-19T21:36:00Z

synapseclient/models/project.py

+        copy_annotations: bool = True,
+        copy_wiki: bool = True,
+        exclude_types: Optional[List[str]] = None,
+        file_update_existing: bool = False,


Same comment as above, now that this is on a project level, there's other entities that can be updated beyond files

thomasyu888 · 2024-02-19T21:37:03Z

synapseclient/models/services/search.py

+    can_search = (
+        entity.id
+        or (
+            entity.name and (entity.__class__.__name__ == "Project" or entity.parent_id)


Nit: Is this where you can use isinstance(entity, Project)?

Yes - However I had implemented it this way to prevent circular imports. I could have done a lazy import above this line, but this also works.

…ds (#1068) * Sync interface for all async methods

Project model finishing touches - also folder is_restricted

c5e1811

BryanFauble added 6 commits February 14, 2024 13:50

Bug busting and creating the project script

ba5e801

Bug fixes from unit testing

a2b88e9

Unit test updated folder logic when there is/is not a change in content

e6e44ed

Bug fix caught by integration tests

fc2c45f

Integration/unit tests for Project. Adding copy function. Fix annotat…

62aa900

…ion copy

Unit test resolutions

31af40a

BryanFauble changed the title ~~DRAFT: [SYNPY-1414] Project model finishing touches~~ [SYNPY-1414] Project model finishing touches Feb 15, 2024

BryanFauble marked this pull request as ready for review February 15, 2024 16:13

BryanFauble requested a review from a team as a code owner February 15, 2024 16:13

BryanFauble added 2 commits February 15, 2024 09:34

Sonarcloud resolutions

50e4c11

Sonar resolutions

d121a99

BWMac reviewed Feb 15, 2024

View reviewed changes

docs/scripts/object_orientated_programming_poc/oop_poc_project.py Outdated Show resolved Hide resolved

BWMac reviewed Feb 15, 2024

View reviewed changes

synapseclient/client.py Show resolved Hide resolved

synapseclient/models/project.py Show resolved Hide resolved

BryanFauble added 3 commits February 15, 2024 10:42

Adding note about debug level

0a0f977

Added note about debug flag

b0a740c

Remove check in storable_container as it happens in the .get() call

2f6eeed

BWMac self-requested a review February 19, 2024 17:07

BWMac approved these changes Feb 19, 2024

View reviewed changes

thomasyu888 reviewed Feb 19, 2024

View reviewed changes

docs/scripts/object_orientated_programming_poc/oop_poc_project.py Show resolved Hide resolved

thomasyu888 reviewed Feb 19, 2024

View reviewed changes

thomasyu888 approved these changes Feb 19, 2024

View reviewed changes

[SYNPY-1433] Synchronous wrappers for all external facing async metho…

e8f23f7

…ds (#1068) * Sync interface for all async methods

BryanFauble merged commit 329849d into develop Feb 21, 2024
27 of 28 checks passed

BryanFauble deleted the SYNPY-1414-project-model-finishing-touches branch February 21, 2024 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYNPY-1414] Project model finishing touches #1067

[SYNPY-1414] Project model finishing touches #1067

BryanFauble commented Feb 13, 2024 •

edited

pep8speaks commented Feb 13, 2024 •

edited

sonarcloud bot commented Feb 15, 2024

thomasyu888 Feb 19, 2024

BryanFauble Feb 19, 2024

thomasyu888 Feb 19, 2024 •

edited

BryanFauble Feb 19, 2024

thomasyu888 Feb 19, 2024 •

edited

BryanFauble Feb 19, 2024

thomasyu888 left a comment

thomasyu888 Feb 19, 2024

BryanFauble Feb 19, 2024

thomasyu888 Feb 19, 2024

thomasyu888 Feb 19, 2024

thomasyu888 Feb 19, 2024

thomasyu888 Feb 19, 2024

BryanFauble Feb 19, 2024



		def merge_dataclass_entities(
		source: typing.Union["Project", "Folder"],

[SYNPY-1414] Project model finishing touches #1067

[SYNPY-1414] Project model finishing touches #1067

Conversation

BryanFauble commented Feb 13, 2024 • edited

pep8speaks commented Feb 13, 2024 • edited

Comment last updated at 2024-02-15 21:20:43 UTC

sonarcloud bot commented Feb 15, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasyu888 Feb 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasyu888 Feb 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasyu888 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanFauble commented Feb 13, 2024 •

edited

pep8speaks commented Feb 13, 2024 •

edited

thomasyu888 Feb 19, 2024 •

edited

thomasyu888 Feb 19, 2024 •

edited