Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inject a GetOrCreateMixin to Node and Relationship. #244

Merged
merged 20 commits into from
Sep 20, 2023

Conversation

aalekhpatel07
Copy link
Contributor

@aalekhpatel07 aalekhpatel07 commented May 11, 2023

Description

Add a Django-like get_or_create() shortcut for Node and Relationship to simplify the following idiom of merging nodes and relationships:

Before:

try:
    streamer = Streamer(id="3").load(db=db)
except:
    print("Creating new Streamer node in the database.")
    streamer = Streamer(id="3", username="anne", followers=222).save(db=db)

After:

streamer, created = Streamer(id="3").get_or_create(db=db)
# only update if its created.
if created:
    streamer.username = "anne"
    streamer.followers = 222
    streamer.save(db=db)

Pull request type

  • Feature
  • Refactoring with functional or API changes

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

######################################

Reviewer checklist (the reviewer checks this part)

  • Core feature implementation
  • Tests
  • Code documentation
  • Documentation on memgraph/docs

######################################

@aalekhpatel07 aalekhpatel07 changed the base branch from main to develop May 11, 2023 18:27
@katarinasupe
Copy link
Contributor

katarinasupe commented May 15, 2023

Hi @aalekhpatel07, thank you for the contribution. We will check it asap, and get back to you. For now, can you rebase to main, since we changed our development process (I updated the Contribution guide now)?

@aalekhpatel07 aalekhpatel07 changed the base branch from develop to main May 15, 2023 15:54
@aalekhpatel07
Copy link
Contributor Author

The changelist should be atomic and more manageable now.

@katarinasupe katarinasupe added the type: enhancement New feature or request label May 31, 2023
@katarinasupe
Copy link
Contributor

Hi @aalekhpatel07 :) I managed to look at the PR, and it all seems good. Can you maybe add a test (similar to what you provided above) here? If not, please let me add commits if you didn't allow it before. Again, thanks for the contribution.

Copy link
Contributor

@katarinasupe katarinasupe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can, please add at least one test. Ideally, check the procedure both for node and relationship object.

…ementations for improved docs.

Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
@aalekhpatel07
Copy link
Contributor Author

aalekhpatel07 commented Jun 1, 2023

I've added a couple of tests.

Unfortunately, pymgclient is broken for me, so poetry install is a bit of a pain because I have to install pymygclient separately.

I'll let CI be the judge for these tests.

Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
…"name" to the node instantiation because it is a required field.
@aalekhpatel07
Copy link
Contributor Author

aalekhpatel07 commented Jun 12, 2023

Thank you for approving the CI run. that helped spot a couple of tiny mistakes. Lets try running it again? This might be a couple of iterations since CI is the only feedback for me, but we'll get there soon.

@katarinasupe
Copy link
Contributor

You can also test it locally with
poetry run pytest . -k "not slow and not extras"
and running Memgraph instance. The only tests failing then should be Neo4j ones.

…user-defined `id`.

Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
Signed-off-by: Aalekh Patel <aalekh.gwpeck.7998@icloud.com>
@aalekhpatel07
Copy link
Contributor Author

Okay, all test_get_or_create tests are passing now. 🥳 There's a spurious failing test tests/graph_algorithms/test_query_builder.py::test_memgraph_query_builder_methods_exist that doesn't seem related to my changelist.

I went with setting up a neo4j and a memgraph instance in the background while running the tests locally:

docker run \
    --name neo4j \
    -p 7688:7687 \
    -p 7474:7474 \
    -e NEO4J_AUTH="neo4j/test" \
    --rm \
    -it neo4j:4.4.7
docker run \
    --name memgraph \
    -p 7687:7687 \
    -p 3000:3000 \
    --rm \
    -it memgraph
poetry run pytest . -k 'not slow and not extras'

@katarinasupe
Copy link
Contributor

Hi @aalekhpatel07, your test are passing now! Sorry for being a bit late on running them. We will push this PR to the next release. I will update you on when that is. Is our release timeline a blocker for you?

@katarinasupe katarinasupe added this to the v1.5.0 milestone Jun 21, 2023
@katarinasupe katarinasupe removed the request for review from brunos252 June 21, 2023 08:51
@aalekhpatel07
Copy link
Contributor Author

Not really. Next release seems reasonable. It's mostly a qol improvement at this point.

@katarinasupe katarinasupe added the status: ship it PR approved label Jul 3, 2023
@katarinasupe katarinasupe merged commit 0fe2368 into memgraph:main Sep 20, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ship it PR approved type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants