zhangjiayin / caddy-geoip2

caddy-geoip2
21 stars 3 forks source link

Support for Subdivisions #3

Closed NCRonB closed 1 year ago

NCRonB commented 1 year ago

I would love to see support for Subdivisions. In the US, this provides access to states.

zhangjiayin commented 1 year ago

I've create a PR for it https://github.com/zhangjiayin/caddy-geoip2/pull/4

but I do not know how to use an array or map in caddyfile?

could confirm that is it Useful for you ?

after that I'll merge it .

zhangjiayin commented 1 year ago

and the PR have been merged check it,Please.

NCRonB commented 1 year ago

I haven't tested it yet, but it looks good. One concern is the placeholder names. If I'm reading the code right, the subdivision names are inconsistent with the other names. For example:

Current placeholders for city:

geoip2.city_geoname_id
geoip2.city_name

Current placeholders for subdivisons:

  geoip2.subdivisions.1.iso_code
  geoip2.subdivisions.1.name

It would be more consistent to use underscores:

  geoip2.subdivision_1_iso_code
  geoip2.subdivision_1_name

That is what MaxMind uses in their CSV files.

However, I think it would be even better to use dot-separation for all of them like other Caddy placeholders use to represent a struct. For example:

  geoip2.representedcountry.iso_code
  geoip2.representedcountry.name

  geoip2.city.geoname_id
  geoip2.city.name

  geoip2.location.latitude
  geoip2.location.longitude
  geoip2.location.time_zone

  geoip2.subdivision.1.iso_code
  geoip2.subdivision.1.name

It's up to you, of course, but this looks like a good way to match the MaxMind structure in Caddy.

zhangjiayin commented 1 year ago

You are right. it is more consistent to use underscores.

https://github.com/zhangjiayin/caddy-geoip2/pull/5

zhangjiayin commented 1 year ago

I've merge the change. so this issue would be Closed

NCRonB commented 1 year ago

It's working mostly as expected. However, the index starts at zero instead of one, so the first subdivision is geoip2.subdivisions.0.name, which is not consistent with MaxMind.

zhangjiayin commented 1 year ago

OK I've made a change https://github.com/zhangjiayin/caddy-geoip2/pull/6 to follow you suggestions.

Thank you.

NCRonB commented 1 year ago

Seems to be working well. Thanks!