[DO NOT MERGE] Code review #53

jairad26 · 2025-01-03T19:43:11Z

No description provided.

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.29.0 to 0.31.0. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/golang/crypto/commit/b4f1988a35dee11ec3e05d6bf3e90b695fbd8909"><code>b4f1988</code></a> ssh: make the public key cache a 1-entry FIFO cache</li> <li><a href="https://github.com/golang/crypto/commit/7042ebcbe097f305ba3a93f9a22b4befa4b83d29"><code>7042ebc</code></a> openpgp/clearsign: just use rand.Reader in tests</li> <li><a href="https://github.com/golang/crypto/commit/3e90321ac7bcee3d924ed63ed3ad97be2079cb56"><code>3e90321</code></a> go.mod: update golang.org/x dependencies</li> <li><a href="https://github.com/golang/crypto/commit/8c4e668694ccbaa1be4785da7e7a40f2ef93152b"><code>8c4e668</code></a> x509roots/fallback: update bundle</li> <li>See full diff in <a href="https://github.com/golang/crypto/compare/v0.29.0...v0.31.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=golang.org/x/crypto&package-manager=go_modules&previous-version=0.29.0&new-version=0.31.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/hypermodeinc/modusDB/network/alerts). </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…ort (#35)

This PR adds the initial issue and PR templates, a changelog, code of conduct, contributing guide, and security reporting instructions. We can enhance these over time.

**Description** Initialize Trunk for broader linting and security monitoring. Fix addressable issues. **Checklist** - [x] Code compiles correctly and linting passes locally

**Description** add readme to repo

**Description** Adds copyrights to all go files **Checklist** - [x] Code compiles correctly and linting passes locally - [ ] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here Thank you for your contribution to modusDB!

**Description** Move GitHub Actions workflows to Warp runners for faster tests through larger machine access. **Checklist** - [X] Code compiles correctly and linting passes locally

**Description** This PR adds support for reverse edges via the readFrom json tag **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

**Description** refactoring packages for clearer code, remove duplications, etc **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [ ] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

**Description** more refactoring to simplify code in main modusdb package **Checklist** - [x] Code compiles correctly and linting passes locally

mangalaman93

I also do not understand the package structure having two levels of hierarchy with just one file in each package. This does not feel useful to me. packages are for dividing code for logically structuring it. I don't see that here.

This is a 10000 feet view of the PR. I will do another pass once some of these comments are addressed. This is a very long PR to review.

mangalaman93 · 2025-01-06T06:29:05Z

api/mutations/mutations.go

+	"github.com/hypermodeinc/modusdb/api/utils"
+)
+
+func HandleReverseEdge(jsonName string, value reflect.Type, nsId uint64, sch *schema.ParsedSchema,


we should move these to internal package if we want to have this package structure.

api/mutations/mutations.go

mangalaman93 · 2025-01-06T06:31:01Z

api/query_gen/dql_query.go

+	return fmt.Sprintf(ObjQuery, qf(), extraFields)
+}
+
+func FormatObjsQuery(typeName string, qf QueryFunc, paginationAndSorting string, extraFields string) string {


what is the benefit of passing a function qf instead of passing the return value here?

formatObjsQuery doesn't need queryfunc, its there to maintain code readability bc formatObjQuery does need it. it makes more sense if they both follow using qf

api/utils/reflect.go

api/utils/utils.go

mangalaman93 · 2025-01-06T06:34:34Z

api_mutation_helpers.go

+	return gid, nil
+}
+
+func applyDqlMutations(ctx context.Context, db *DB, dms []*dql.Mutation) error {


All the functions that do mutations and query or alter or any other Dgraph operation, that should use moduDB APIs or equivalent so that we can make changes to it easily.

…is specifically

**Description** On query of a type with a reverse edge, it modifies the type to remove all other predicates. The expected logic should only alter the schema if the type and predicate for the reverse edge does not exist. **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

mattjohnsonpint · 2025-01-08T05:49:55Z

.github/ISSUE_TEMPLATE/bug_report.md

+**Describe the bug** A clear and concise description of what the bug is.
+
+**To Reproduce** Steps to reproduce the behavior:
+
+1. Go to '...'
+2. Click on '....'
+3. Scroll down to '....'
+4. See error
+
+**Expected behavior** A clear and concise description of what you expected to happen.
+
+**Screenshots** If applicable, add screenshots to help explain your problem.
+
+**Environment**
+
+- OS: [e.g. macOS, Windows, Ubuntu]
+- Language [e.g. Go]
+- Version [e.g. v0.xx]
+
+**Additional context** Add any other context about the problem here.


Trunk merged the title and body of these paragraphs, because they were just bold text rather than headings. Use ## headings instead.

mattjohnsonpint · 2025-01-08T05:53:09Z

.github/ISSUE_TEMPLATE/feature_request.md

+**Is your feature request related to a problem? Please describe.** A clear and concise description
+of what the problem is. Ex. I'm always frustrated when [...]
+
+**Describe the solution you'd like** A clear and concise description of what you want to happen.
+
+**Describe alternatives you've considered** A clear and concise description of any alternative
+solutions or features you've considered.
+
+**Additional context** Add any other context or screenshots about the feature request here.


Trunk merged the title and body of these paragraphs, because they were just bold text rather than headings. Use ## headings instead.

mattjohnsonpint · 2025-01-08T05:58:09Z

CONTRIBUTING.md

Consider adding a section about Trunk. Feel free to copy from the one I did in Modus.

mattjohnsonpint · 2025-01-08T05:58:25Z

.github/pull_request_template.md

This looks like it was copied from the Modus repo, but I've recently changed the text there. Please consider updating to match. (At minimum, update the headings to use ## markdown.)

mattjohnsonpint · 2025-01-08T06:03:39Z

.github/workflows/ci-go-lint.yaml

+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version-file: go.mod


Unfortunately, setup-go when using go-version-file does not recognize the toolchain directive in the file. See: actions/setup-go#457

That means it will install the version from the go directive (1.22.7), not 1.23.x that you might expect.

So instead, configure manually and keep it updated.

Suggested change

go-version-file: go.mod

go-version: 1.23.4

mattjohnsonpint · 2025-01-08T17:43:35Z

.trunk/configs/.markdownlint.json

Since this is in trunk's configs dir, you'll be fighting against VS Code expecting it in the root. Trunk will have this config, and VS Code will have the default config.

You can fix that by adding the following to .vscode/settings.json

"markdownlint.config": { "extends": ".trunk/configs/.markdownlint.json" },

mattjohnsonpint · 2025-01-08T17:47:29Z

CHANGELOG.md

+- feat: add readfrom json tag to support reverse edges
+  [#49](https://github.com/hypermodeinc/modusDB/pull/49)
+
+- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)


Suggested change

- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)

- chore: Refactoring package management [#51](https://github.com/hypermodeinc/modusDB/pull/51)

mattjohnsonpint · 2025-01-08T17:50:08Z

CONTRIBUTING.md

+
+If in doubt or contemplating a larger change, join the
+[Hypermode Discord](https://discord.hypermode.com) and start a discussion in the
+[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.


Suggested change

[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.

[#modusdb](https://discord.com/channels/1267579648657850441/1324490523725135872) channel.

mattjohnsonpint · 2025-01-08T17:55:48Z

README.md

+}
+
+func main() {
+  db, err := New(NewDefaultConfig("/tmp/modusdb"))


Suggested change

db, err := New(NewDefaultConfig("/tmp/modusdb"))

cfg := modusdb.NewDefaultConfig("/tmp/modusdb")

db, err := modusdb.New(cfg)

mattjohnsonpint · 2025-01-08T18:01:10Z

README.md

+  }
+  fmt.Println(user)
+
+  _, queriedUser, err := modusdb.Get[User](db, gid)


It's unclear what the _ return value is. If it's the gid, why does the API accept a gid and also return one? Could they differ somehow?

mattjohnsonpint · 2025-01-10T01:00:34Z

api.go

+	"github.com/hypermodeinc/modusdb/api/apiutils"
+	"github.com/hypermodeinc/modusdb/api/structreflect"
+)
+


Please add usage comments for at least these primary public functions. Otherwise, it's difficult to tell what the parameters and return values are supposed to be. Thanks.

mattjohnsonpint · 2025-01-10T01:10:15Z

live_test.go

These tests take too long. Please separate this and other long-running integration tests using build tags. Thanks.

mattjohnsonpint · 2025-01-10T01:16:50Z

api.go

+	if len(ns) > 1 {
+		return nil, nil, fmt.Errorf("only one namespace is allowed")
+	}
+	ctx, n, err := getDefaultNamespace(db, ns...)


Here and in the rest of the API methods, we are creating the ctx context object ourselves inside the getDefaultNamespace function. Instead, we should be allowing the user to pass context in as a parameter (usually the first parameter).

For example, say the user wants the query to timeout if it takes to long to run. The normal way would be to use context.WithTimeout and pass that context in to the query function. But we've not given them any way to do that.

... Also, it's unclear why there is Query as a global function that doesn't take a context but takes a db and a namespace, and db.Query which does take a context, but not a namespace.

IMHO, There should just be one way to query, not two.

jairad26 and others added 19 commits December 18, 2024 13:28

Jai/hyp 2765 get api (#28)

bb00fef

separate ci tests and lint (#30)

208821a

Create CODEOWNERS

571b799

add readFrom, index creation, and gid for apis (#31)

0420655

Jai/hyp 2771 benchmark modusdb (#32)

a8aee3e

upgrade dependencies in go.mod (#24)

2e3b106

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

add delete api (#33)

3e824e2

add nested mutation, getting nested values, upsert, nested types supp…

1116c7c

…ort (#35)

support unstructured data in modusdb (#38)

84b781b

chore: repo setup (#43)

f7007a4

This PR adds the initial issue and PR templates, a changelog, code of conduct, contributing guide, and security reporting instructions. We can enhance these over time.

chore: create LICENSE (#42)

fff8aaa

chore: add trunk (#44)

e676bc6

**Description** Initialize Trunk for broader linting and security monitoring. Fix addressable issues. **Checklist** - [x] Code compiles correctly and linting passes locally

chore: add readme (#46)

a157525

**Description** add readme to repo

Update trunk.yaml

77effe0

chore: move to Warp runners for GitHub Actions (#50)

2e7ac26

**Description** Move GitHub Actions workflows to Warp runners for faster tests through larger machine access. **Checklist** - [X] Code compiles correctly and linting passes locally

jairad26 requested review from mangalaman93 and mattjohnsonpint January 3, 2025 19:43

chore: move reflection logic to utils package (#54)

7abdc72

**Description** more refactoring to simplify code in main modusdb package **Checklist** - [x] Code compiles correctly and linting passes locally

mangalaman93 requested changes Jan 6, 2025

View reviewed changes

jairad26 added 5 commits January 6, 2025 00:19

revs

48b27de

.

205eaf7

rename internal to api to reflect the functions inside are for the ap…

f24f940

…is specifically

Merge branch 'main' into code-review

b9226a5

mattjohnsonpint reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] Code review #53

[DO NOT MERGE] Code review #53

jairad26 commented Jan 3, 2025

mangalaman93 left a comment

mangalaman93 Jan 6, 2025

mangalaman93 Jan 6, 2025

jairad26 Jan 6, 2025

mangalaman93 Jan 6, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 8, 2025

mattjohnsonpint Jan 10, 2025

mattjohnsonpint Jan 10, 2025 •

edited

Loading

mattjohnsonpint Jan 10, 2025

mattjohnsonpint Jan 10, 2025

	- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)
	- chore: Refactoring package management [#51](https://github.com/hypermodeinc/modusDB/pull/51)

	[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.
	[#modusdb](https://discord.com/channels/1267579648657850441/1324490523725135872) channel.

	db, err := New(NewDefaultConfig("/tmp/modusdb"))
	cfg := modusdb.NewDefaultConfig("/tmp/modusdb")
	db, err := modusdb.New(cfg)

[DO NOT MERGE] Code review #53

Are you sure you want to change the base?

[DO NOT MERGE] Code review #53

Conversation

jairad26 commented Jan 3, 2025

mangalaman93 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

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

mattjohnsonpint Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjohnsonpint Jan 10, 2025 •

edited

Loading