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

Raise a helpful error if Natural Earth data is not imported #1220

Closed
ZeLonewolf opened this issue Sep 10, 2021 · 8 comments · Fixed by openmaptiles/openmaptiles-tools#370 or #1236
Closed

Comments

@ZeLonewolf
Copy link
Contributor

During testing of #1213, it was noted that running make import-sql without having previously run make import-data causes a somewhat cryptic error message. Instead, we can add a check to the top of the SQL to verify that the Natural Earth tables are present, and raise a more helpful error message if they're missing.

@nyurik
Copy link
Member

nyurik commented Sep 10, 2021

We could also extend the yaml files to say which tables or functions each layer expects to be present in the DB before it is imported. This way we can auto-generate the same checks for all layers, and prepend them to the sql files.

@ZeLonewolf
Copy link
Contributor Author

Hi, does openmaptiles/openmaptiles-tools#370 actually fix this issue? Or are more changes needed on the openmaptiles side to update various yaml files with the Natural Earth dependencies?

@Falke-Design
Copy link
Contributor

No this is not fixed. We provided only the funcunality now we need to add the required things in the layer.yaml

@ZeLonewolf
Copy link
Contributor Author

@TomPohys please re-open ticket.

@nyurik nyurik reopened this Sep 18, 2021
@Falke-Design
Copy link
Contributor

@nyurik now we had added a check for the required tables / functions but it is still no helpful error message:
grafik

It would be better if we show a message like You need first to create the table with 'make import-data'

@nyurik
Copy link
Member

nyurik commented Sep 18, 2021

Good point @Falke-Design, i guess we could add a generic error message at the top (simpler than to have a help text for each required table, and deal with duplicate messages, etc).

requires:
  helpText: "This layer requires tables generated with 'make import-data' command."
  tables:
    - osm_ocean_polygon
    ...

I guess we could wrap the currently-generated code with something like this:

do $$
begin
  SELECT 'osm_ocean_polygon'::REGCLASS;
exception when others then   -- I think we should use a specific error instead of "others" here
  RAISE EXCEPTION SQLERRM USING HINT = 'This layer requires tables generated with "make import-data" command.' ;
end;
$$ language 'plpgsql';

I think SQLERRM is what contains statement-specific error, and HINT will help with additional information. I haven't tested the above. Also note that hint will have to be quote-escaped.

P.S. Maybe it is better to use DETAIL instead of HINT

TomPohys pushed a commit to openmaptiles/openmaptiles-tools that referenced this issue Oct 8, 2021
Add `helpText` to `layer.requires` like discussed in openmaptiles/openmaptiles#1220

I had to change from `SELECT 'osm_ocean_polygon'::regclass;` to `PERFORM 'osm_ocean_polygon'::regclass;` because plpgsql needs a return value if `SELECT` is used.

Also added an error if a function has no arguments:
```
when invalid_text_representation then
        RAISE EXCEPTION '%! The arguments of the required function "osmFunc" of the layer "water" are missing. Example: "osmFunc(text)"', SQLERRM;
```

Example:
```
layer:
  id: "water"
  requires:
    helpText: 'This is a text with "quotes"'
    functions: "osmFunc"
    tables: "osm_ocean_polygon"
```

SQL:
```
-- Assert osm_ocean_polygon exists
do $$
begin
        PERFORM 'osm_ocean_polygon'::regclass;
exception when undefined_table then
        RAISE EXCEPTION '%! This is error text with "quotes"', SQLERRM;
end;
$$ language 'plpgsql';

-- Assert osmFunc exists
do $$
begin
        PERFORM 'osmFunc'::regprocedure;
exception when undefined_function then
        RAISE EXCEPTION '%! This is a text with "quotes"', SQLERRM;
when invalid_text_representation then
        RAISE EXCEPTION '%! The arguments of the required function "osmFunc" of the layer "water" are missing. Example: "osmFunc(text)"', SQLERRM;
end;
$$ language 'plpgsql';
```

Result of `make import-sql`:
> psql:/sql/parallel/water__waterway.sql:10: ERROR:  relation "osm_ocean_polygon2" does not exist! This is a text with "quotes"

Co-authored-by: Yuri Astrakhan <YuriAstrakhan@gmail.com>
@Falke-Design
Copy link
Contributor

@lazaa32 this is not fixed. The functions are now available but the error / help messages are still missing

@lazaa32
Copy link
Collaborator

lazaa32 commented Oct 22, 2021

@Falke-Design sorry, weird, closed by accident when syncing my fork. Reopening.

@lazaa32 lazaa32 reopened this Oct 22, 2021
TomPohys pushed a commit that referenced this issue Nov 5, 2021
**NOTE** this can only be merged after the next tools version is released.

Added required Postgres tables to the `<layer>.yaml` definition.
Close: #1220

PR of tools: openmaptiles/openmaptiles-tools#370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants