Multi Schema Support - #1
Conversation
rkrishnasanka
left a comment
rkrishnasanka
left a comment
There was a problem hiding this comment.
@aquaticcalf - Please make sure you add code documentation for every function that you're touching (this should be even for the function doc).
There was a problem hiding this comment.
Change of to Of to make it camel case compatible. Additionally save this as a constant value in the go file incase we want to make this editable in the future.
There was a problem hiding this comment.
okay, that will be done
and camel case as in, do you want it to be tableOfSchema?
like the schema name should also be starting with a capital letter?
| edgesIndex map[string][]edgeInfo // edges index | ||
| allEdges map[int32]TEdge // all edges | ||
| relationshipGraph *util.Graph // relationship graph | ||
| allowedSchemas map[string]bool // track allowed schemas |
There was a problem hiding this comment.
This is question, why are we keeping this as a map rather than a list. Aren't with implicitly stating that all the allowedSchemas are true ?
There was a problem hiding this comment.
right, not sure what i was thinking when i took that decision..
yes this sound good, a list rather than a map
| pageInfoType := FullType{ | ||
| Kind: "OBJECT", | ||
| Name: "PageInfo", | ||
| Description: "Information about pagination in a connection", | ||
| Fields: []FieldObject{ | ||
| { | ||
| Name: "hasNextPage", | ||
| Description: "Indicates if there are more pages after the current page", | ||
| Type: newTypeRef("NON_NULL", "", newTypeRef("SCALAR", "Boolean", nil)), | ||
| Args: []InputValue{}, | ||
| }, | ||
| { | ||
| Name: "hasPreviousPage", | ||
| Description: "Indicates if there are pages before the current page", |
There was a problem hiding this comment.
Are we addressing a specific problem with this update related to multi-schema or is it a general fix ?
There was a problem hiding this comment.
oops, that code isn't supposed to be in this branch..
i was playing with the library, learning a few new thing i didnt know before
so that's where all the other stuff is from
will remove!
| // For tables not in the default schema, add them with "tablenameofschemaname" format | ||
| if t.Schema != defaultSchema && in.schema.IsAllowedSchema(t.Schema) { | ||
| tableCopy := t | ||
| crossSchemaAlias := t.Name + "of" + t.Schema |
There was a problem hiding this comment.
Its better to pull this from some kind of a global constant
There was a problem hiding this comment.
okay, Of will be a global constant now
i will autocapitalise it, so that even if the value in the constant is "of" , we do "Of"
There was a problem hiding this comment.
There's a lot of changes here. What is the purpose of these changes @aquaticcalf ?
There was a problem hiding this comment.
will add documentation in the next iteration
| allowedSchemas map[string]bool // track allowed schemas | ||
| allowedSchemas []string // allowed schemas | ||
| defaultSchema string // default schema | ||
| enableCamelCase bool // enable camel case for cross schema names |
There was a problem hiding this comment.
I think using camelCase is the norm for graphql.
| DefaultSchema string // Default schema to use | ||
| AllowedSchemas []string // List of allowed schemas | ||
| DefaultSchema string // Default schema to use | ||
| EnableCamelCase bool // Enable camel case for cross schema names |
There was a problem hiding this comment.
I'm trying to decide if this the addition of this level of configuration is necessary. I think people would rather just pass the default separator here instead of wondering if its camelcase or not
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| if c.Core.DBType == "" { | ||
| c.Core.DBType = c.DB.Type | ||
| if c.DB.Type == "" { | ||
| c.DB.Type = c.DB.Type |
There was a problem hiding this comment.
Bug: Self-assignment has no effect on config initialization
The line c.DB.Type = c.DB.Type is a self-assignment that does nothing. The comment says "copy over db_type from database.type" but the code assigns the variable to itself. This appears to be an incomplete migration from an older pattern where Core.DBType was being copied to DB.Type. As written, if c.DB.Type is empty, it stays empty.
| return strings.TrimSpace(parts[0]), strings.TrimSpace(parts[1]) | ||
| } | ||
| return name, co.s.DefaultSchema() | ||
| } |
There was a problem hiding this comment.
Bug: Hardcoded separator mismatches configurable separator value
The parseTableName function uses a hardcoded lowercase "of" separator, while GetCrossSchemaSeparator() defaults to capital "Of". Similarly, intro.go uses lowercase "of" when building cross-schema names. This case mismatch means table names formatted with the default "Of" separator (e.g., "tableOfpublic") won't be correctly parsed since the split looks for lowercase "of". The function should use co.s.GetCrossSchemaSeparator() instead.
Additional Locations (2)
|
|
||
| if err = addForeignKeys(gj.conf, gj.dbinfo); err != nil { | ||
| return | ||
| gj.schema, err = sdata.NewDBSchema(gj.dbinfo, nil, schemaConfig) |
There was a problem hiding this comment.
Bug: Table aliases not passed to schema initialization
The NewDBSchema call passes nil for the aliases parameter instead of getDBTableAliases(gj.conf). The old code passed the configured table aliases to allow users to define alternative names for tables. With nil, any table aliases defined in the configuration will be silently ignored, breaking alias functionality for users who rely on it.
| gj.log.Printf("WARNING: dbinfo is nil in initSchema") | ||
| } else { | ||
| gj.log.Printf("DEBUG: dbinfo has %d tables", len(gj.dbinfo.Tables)) | ||
| } |
There was a problem hiding this comment.
Bug: Debug logging statements accidentally left in production code
Multiple DEBUG: and WARNING: log statements have been left in the production code. The PR discussion confirms this was unintentional, with the author stating "that code isn't supposed to be in this branch.. i was playing with the library, learning a few new things". These debug statements will clutter production logs with internal implementation details.
Additional Locations (2)
2 participants
Note
Adds multi-schema support across config, schema, compilers, SQL generation, and introspection, including schema-qualified SQL and cross-schema GraphQL aliases, with templates and services updated to use the new database config.
Config.Database(DatabaseConfig) with connection/pool/TLS andschemas(allowed,default,separator).database.typeand pass schema config tosdata.NewDBSchema.sdata):DBSchemawithallowedSchemas,defaultSchema, and cross-schema separator; utilities:DefaultSchema,IsAllowedSchema,GetCrossSchemaSeparator, parsing helpers, qualifiedFindbehavior.qcode):tableOfschema), schema validation, and usesDefaultSchemawhen unset.psql):DefaultSchemato config; renders schema-qualified tables and quotes identifiers viaQuoteIdent.tablenameofschemaname) and corresponding Query/Mutation fields.dev.yml/prod.yml: adddatabase.schemasblock.conf.DB.Type/conf.Database.Typein admin, service, and WASM paths.sdata.NewDBSchemasignature and add separator tests.Written by Cursor Bugbot for commit 4844e1a. This will update automatically on new commits. Configure here.