Fix some ORM model generator issues.

vpetrovykh · vpetrovykh · commit 2c4ec147c882 · 2024-12-22T21:48:02.000-05:00
Instead of hard errors issue warnings and attempt to skip over features
that cannot be implemented. A partial working set of models is better
than none and sometimes is what is needed.

Stop allowing multi properties in reflected models, they fundamentally
lack an identity and don't mesh with the Object models.
diff --git a/gel/_testbase.py b/gel/_testbase.py
@@ -33,11 +33,12 @@
 import tempfile
 import time
 import unittest
+import warnings
 
 import gel
 from gel import asyncio_client
 from gel import blocking_client
-from gel.orm.introspection import get_schema_json
+from gel.orm.introspection import get_schema_json, GelORMWarning
 from gel.orm.sqla import ModelGenerator as SQLAModGen
 from gel.orm.django.generator import ModelGenerator as DjangoModGen
 
@@ -646,17 +647,20 @@ def setUpClass(cls):
         if importlib.util.find_spec("psycopg2") is None:
             raise unittest.SkipTest("need psycopg2 for ORM tests")
 
-        super().setUpClass()
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", GelORMWarning)
 
-        class_set_up = os.environ.get('EDGEDB_TEST_CASES_SET_UP')
-        if not class_set_up:
-            # We'll need a temp directory to setup the generated Python
-            # package
-            cls.tmpormdir = tempfile.TemporaryDirectory()
-            sys.path.append(cls.tmpormdir.name)
-            # Now that the DB is setup, generate the ORM models from it
-            cls.spec = get_schema_json(cls.client)
-            cls.setupORM()
+            super().setUpClass()
+
+            class_set_up = os.environ.get('EDGEDB_TEST_CASES_SET_UP')
+            if not class_set_up:
+                # We'll need a temp directory to setup the generated Python
+                # package
+                cls.tmpormdir = tempfile.TemporaryDirectory()
+                sys.path.append(cls.tmpormdir.name)
+                # Now that the DB is setup, generate the ORM models from it
+                cls.spec = get_schema_json(cls.client)
+                cls.setupORM()
 
     @classmethod
     def setupORM(cls):
diff --git a/gel/orm/cli.py b/gel/orm/cli.py
@@ -18,11 +18,12 @@
 
 
 import argparse
+import warnings
 
 import gel
 
 from gel.codegen.generator import _get_conn_args
-from .introspection import get_schema_json
+from .introspection import get_schema_json, GelORMWarning
 from .sqla import ModelGenerator as SQLAModGen
 from .django.generator import ModelGenerator as DjangoModGen
 
@@ -73,8 +74,15 @@ def main():
     args = parser.parse_args()
     # setup client
     client = gel.create_client(**_get_conn_args(args))
-    spec = get_schema_json(client)
-    generate_models(args, spec)
+
+    with warnings.catch_warnings(record=True) as wlist:
+        warnings.simplefilter("always", GelORMWarning)
+
+        spec = get_schema_json(client)
+        generate_models(args, spec)
+
+        for w in wlist:
+            print(w.message)
 
 
 def generate_models(args, spec):
diff --git a/gel/orm/django/generator.py b/gel/orm/django/generator.py
@@ -1,7 +1,8 @@
 import pathlib
 import re
+import warnings
 
-from ..introspection import get_mod_and_name, FilePrinter
+from ..introspection import get_mod_and_name, GelORMWarning, FilePrinter
 
 
 GEL_SCALAR_MAP = {
@@ -24,7 +25,7 @@
     'cal::local_date': 'DateField',
     'cal::local_datetime': 'DateTimeField',
     'cal::local_time': 'TimeField',
-    # all kinds of duration is not supported due to this error:
+    # all kinds of durations are not supported due to this error:
     # iso_8601 intervalstyle currently not supported
 }
 
@@ -53,7 +54,6 @@ class GelPGMeta:
     'This is a model reflected from Gel using Postgres protocol.'
 '''
 
-FK_RE = re.compile(r'''models\.ForeignKey\((.+?),''')
 CLOSEPAR_RE = re.compile(r'\)(?=\s+#|$)')
 
 
@@ -83,19 +83,16 @@ def __init__(self, *, out):
 
     def spec_to_modules_dict(self, spec):
         modules = {
-            mod: {} for mod in sorted(spec['modules'])
+            mod: {'link_tables': {}, 'object_types': {}}
+            for mod in sorted(spec['modules'])
         }
 
         for rec in spec['link_tables']:
             mod = rec['module']
-            if 'link_tables' not in modules[mod]:
-                modules[mod]['link_tables'] = {}
             modules[mod]['link_tables'][rec['table']] = rec
 
         for rec in spec['object_types']:
             mod, name = get_mod_and_name(rec['name'])
-            if 'object_types' not in modules[mod]:
-                modules[mod]['object_types'] = {}
             modules[mod]['object_types'][name] = rec
 
         return modules['default']
@@ -128,10 +125,12 @@ def build_models(self, maps):
             # process properties as fields
             for prop in rec['properties']:
                 pname = prop['name']
-                if pname == 'id':
+                if pname == 'id' or prop['cardinality'] == 'Many':
                     continue
 
-                mod.props[pname] = self.render_prop(prop)
+                code = self.render_prop(prop)
+                if code:
+                    mod.props[pname] = code
 
             # process single links as fields
             for link in rec['links']:
@@ -142,7 +141,9 @@ def build_models(self, maps):
 
                 lname = link['name']
                 bklink = mod.get_backlink_name(lname)
-                mod.links[lname] = self.render_link(link, bklink)
+                code = self.render_link(link, bklink)
+                if code:
+                    mod.links[lname] = code
 
             modmap[mod.name] = mod
 
@@ -153,7 +154,16 @@ def build_models(self, maps):
             mod.meta['unique_together'] = "(('source', 'target'),)"
 
             # Only have source and target
-            _, target = get_mod_and_name(rec['target'])
+            mtgt, target = get_mod_and_name(rec['target'])
+            if mtgt != 'default':
+                # skip this whole link table
+                warnings.warn(
+                    f'Skipping link {fwname!r}: link target '
+                    f'{rec["target"]!r} is not supported',
+                    GelORMWarning,
+                )
+                continue
+
             mod.links['source'] = (
                 f"LTForeignKey({source!r}, models.DO_NOTHING, "
                 f"db_column='source', primary_key=True)"
@@ -190,8 +200,11 @@ def render_prop(self, prop):
         try:
             ftype = GEL_SCALAR_MAP[target]
         except KeyError:
-            raise RuntimeError(
-                f'Scalar type {target} is not supported')
+            warnings.warn(
+                f'Scalar type {target} is not supported',
+                GelORMWarning,
+            )
+            return ''
 
         return f'models.{ftype}({req})'
 
@@ -201,7 +214,15 @@ def render_link(self, link, bklink=None):
         else:
             req = ', blank=True, null=True'
 
-        _, target = get_mod_and_name(link['target']['name'])
+        mod, target = get_mod_and_name(link['target']['name'])
+
+        if mod != 'default':
+            warnings.warn(
+                f'Skipping link {link["name"]!r}: link target '
+                f'{link["target"]["name"]!r} is not supported',
+                GelORMWarning,
+            )
+            return ''
 
         if bklink:
             bklink = f', related_name={bklink!r}'
@@ -215,23 +236,28 @@ def render_models(self, spec):
         # Check that there is only "default" module
         mods = spec['modules']
         if mods[0] != 'default' or len(mods) > 1:
-            raise RuntimeError(
-                f"Django reflection doesn't support multiple modules or "
-                f"non-default modules."
+            skipped = ', '.join([repr(m) for m in mods if m != 'default'])
+            warnings.warn(
+                f"Skipping modules {skipped}: Django reflection doesn't "
+                f"support multiple modules or non-default modules.",
+                GelORMWarning,
             )
         # Check that we don't have multiprops or link properties as they
         # produce models without `id` field and Django doesn't like that. It
         # causes Django to mistakenly use `source` as `id` and also attempt to
         # UPDATE `target` on link tables.
         if len(spec['prop_objects']) > 0:
-            raise RuntimeError(
-                f"Django reflection doesn't support multi properties as they "
-                f"produce models without `id` field."
+            warnings.warn(
+                f"Skipping multi properties: Django reflection doesn't "
+                f"support multi properties as they produce models without "
+                f"`id` field.",
+                GelORMWarning,
             )
         if len(spec['link_objects']) > 0:
-            raise RuntimeError(
-                f"Django reflection doesn't support link properties as they "
-                f"produce models without `id` field."
+            warnings.warn(
+                f"Skipping link properties: Django reflection doesn't support "
+                f"link properties as they produce models without `id` field.",
+                GelORMWarning,
             )
 
         maps = self.spec_to_modules_dict(spec)
diff --git a/gel/orm/introspection.py b/gel/orm/introspection.py
@@ -2,6 +2,7 @@
 import re
 import collections
 import textwrap
+import warnings
 
 
 INTRO_QUERY = '''
@@ -62,6 +63,10 @@
 CLEAN_NAME = re.compile(r'^[A-Za-z_][A-Za-z0-9_]*$')
 
 
+class GelORMWarning(Warning):
+    pass
+
+
 def get_sql_name(name):
     # Just remove the module name
     name = name.rsplit('::', 1)[-1]
@@ -80,12 +85,16 @@ def get_mod_and_name(name):
     return name.rsplit('::', 1)
 
 
-def check_name(name):
+def valid_name(name):
     # Just remove module separators and check the rest
     name = name.replace('::', '')
     if not CLEAN_NAME.fullmatch(name):
-        raise RuntimeError(
-            f'Non-alphanumeric names are not supported: {name}')
+        warnings.warn(
+            f'Skipping {name!r}: non-alphanumeric names are not supported',
+            GelORMWarning,
+        )
+        return False
+    return True
 
 
 def get_schema_json(client):
@@ -102,6 +111,23 @@ async def async_get_schema_json(client):
     return _process_links(types, modules)
 
 
+def _skip_invalid_names(spec_list, recurse_into=None):
+    valid = []
+    for spec in spec_list:
+        # skip invalid names
+        if valid_name(spec['name']):
+            if recurse_into is not None:
+                for fname in recurse_into:
+                    if fname not in spec:
+                        continue
+                    spec[fname] = _skip_invalid_names(
+                        spec[fname], recurse_into)
+
+            valid.append(spec)
+
+    return valid
+
+
 def _process_links(types, modules):
     # Figure out all the backlinks, link tables, and links with link
     # properties that require their own intermediate objects.
@@ -110,23 +136,20 @@ def _process_links(types, modules):
     link_objects = []
     prop_objects = []
 
+    # All the names of types, props and links are valid beyond this point.
+    types = _skip_invalid_names(types, ['properties', 'links'])
     for spec in types:
-        check_name(spec['name'])
         type_map[spec['name']] = spec
         spec['backlink_renames'] = {}
 
-        for prop in spec['properties']:
-            check_name(prop['name'])
-
     for spec in types:
         mod = spec["name"].rsplit('::', 1)[0]
         sql_source = get_sql_name(spec["name"])
 
         for prop in spec['properties']:
+            name = prop['name']
             exclusive = prop['exclusive']
             cardinality = prop['cardinality']
-            name = prop['name']
-            check_name(name)
             sql_name = get_sql_name(name)
 
             if cardinality == 'Many':
@@ -158,11 +181,10 @@ def _process_links(types, modules):
 
         for link in spec['links']:
             if link['name'] != '__type__':
+                name = link['name']
                 target = link['target']['name']
                 cardinality = link['cardinality']
                 exclusive = link['exclusive']
-                name = link['name']
-                check_name(name)
                 sql_name = get_sql_name(name)
 
                 objtype = type_map[target]
@@ -175,8 +197,6 @@ def _process_links(types, modules):
                     'has_link_object': False,
                 })
 
-                for prop in link['properties']:
-                    check_name(prop['name'])
 
                 link['has_link_object'] = False
                 # Any link with properties should become its own intermediate
diff --git a/gel/orm/sqla.py b/gel/orm/sqla.py
diff --git a/tests/test_sqla_features.py b/tests/test_sqla_features.py
