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

Feat/hasura config #4

Merged
merged 9 commits into from
Apr 21, 2022
Merged

Conversation

852Kerfunkle
Copy link
Contributor

Heya,

alongside the PR I made to dipdup, to allow multiple souces in a hasura instance, here's my PR for fixing/improving the hasura metadata configuration.

It includes:

  • using the metadata API instad of the deprecated query API
  • A source (for specifying the schema/database) and a add_source (to control if the source should be added via pg_add_source) config entry.
  • more hasura metadata structs/definitions

Probably, there's quite a lot more to improve, but this will do for my use case (hopefully).

Please note that you'll also need this commit from https://github.com/tz1and/metadata/commit/f1af98a396161aa287b3bb0281be2be8118d9a79

Thanks!

@852Kerfunkle
Copy link
Contributor Author

852Kerfunkle commented Apr 20, 2022

Added some more stuff to the hasura metadata structs that would get lost on metadata replace otherwise (table customisations, rest endpoints).

Also opens up the possibility to do camel casing on tables and fields.

Please note this commit: https://github.com/tz1and/metadata/commit/9db1e2b0b6c1cba3ca5962a364ff018d8dc0ae9c
I needed to rename the dipdup_head_status view because the same exists also in dipdup. you can resolve this by way of aliases in hasura, but this was just simpler for me.

@852Kerfunkle
Copy link
Contributor Author

852Kerfunkle commented Apr 20, 2022

Also added a way to run arbitrary hasura metadata customisations. Was wondering if I should include in this PR. Now I have.

Please see the related commit to the metadata indexer here: https://github.com/tz1and/metadata/commit/0724746ed388710738b72dfb5b517a7259208465

It's definitely very useful to have. :)

hasura/requests.go Outdated Show resolved Hide resolved
@aopoltorzhicky
Copy link
Member

aopoltorzhicky commented Apr 21, 2022

Ok, I understood what do you wanted to do =)

Firstly, creating interface{} field or arg is considered a bad practice. In your case custom arg in Create function can be of request type. But in order to implement it you have to make request struct public by renaming it to Request.

Secondly, if you want to decode a JSON field which can be one of multiple types (like DatabaseUrl) to a struct, you have to implement a custom UnmarshalJSON func for your particular type. Something like that (I wrote code here without IDE, so that code might be wrong, but it's just for example):

type DatabaseUrl string

type DatabaseUrlFromEnv struct {
  FromEnv string `json:"from_env"`
}

func (d *DatabaseUrl) UnmarshalJSON(data []byte) error {
  type buf DatabaseUrl
  if err := json.Unmarshal(data, (*buf)(d)); err == nil {
    return nil
  }
  var val DatabaseUrlFromEnv
  if err := json.Unmarshal(data, &val); err == nil {
    *d = DatabaseUrl(val.FromEnv)
    return nil
  }

  // process PGConnectionParameters here
  return nil
}

type A struct {
    // some fields here
     DatabaseUrl DatabaseUrl `json:"database_url"`
    // some fields here
}

But generally I think your code should work, the above is my suggestion to your PR so that we can merge it with minimum refactoring afterwards.
Thanks again for your contribution!

@852Kerfunkle
Copy link
Contributor Author

852Kerfunkle commented Apr 21, 2022

Firstly, creating interface{} field or arg is considered a bad practice. In your case custom arg in Create function can be of request type. But in order to implement it you have to make request struct public by renaming it to Request.

Ok, will make that change!

Secondly, if you want to decode a JSON field which can be one of multiple types (like DatabaseUrl) to a struct, you have to implement a custom UnmarshalJSON func for your particular type.

I think it's not needed as DatabaseUrl is unused and only there so it doesn't get lost on rewriting the metadata. I can do it properly, though, if you feel like it's a requirement.

Thanks!

@aopoltorzhicky
Copy link
Member

I think it's not needed as DatabaseUrl is unused and only there so it doesn't get lost on rewriting the metadata. I can do it properly, though, if you feel like it's a requirement.

No, I think it's not required.

@852Kerfunkle
Copy link
Contributor Author

852Kerfunkle commented Apr 21, 2022

Also, thanks for letting me know how to properly decode multiple types. Good to know.

I changed the custom type to []Request (and made versionedRequest private). One thing is maybe that requests could be versioned, but I'm not sure about this. It certainly works for my use case. See the related commit to readCustomHasuraConfigs here: https://github.com/tz1and/metadata/commit/9416ee6c32964505361f62400597e67670311237

I also added a call to Hasura.SetSourceName in Config.Substitute. So Load will work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants