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

Map inet type to InetAddress #1134

Open
maffe opened this issue Mar 5, 2018 · 32 comments

Comments

@maffe
Copy link

commented Mar 5, 2018

I use JPA but sometimes it’s too limited so I use native queries in those cases. But it means I cannot pass a type parameter to ResultSet.getObject(…, …).

I basically use code like this:

final EntityManager em = …;
final Query query = em.createNativeQuery("SELECT '127.0.0.1'::inet");
final Object result = query.getSingleResult();

In this case, result is a PGobject with type inet. It would be nice if the driver would look at the type and create an InetAddress object automatically, so I can just cast result to InetAddress without needing to know about PGobject at all.

@davecramer

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

Well there is no getInetAddress method in JDBC how would you propose this would work ?

@maffe

This comment has been minimized.

Copy link
Author

commented Mar 6, 2018

Currently I get a PGobject when I need InetAddress. The only place a PGobject is constructed seems to be in PgConnection. I think adding these two lines

      } else if (type.equals("inet")) {
          return InetAddress.getByName(value);

before the else would result in the desired behaviour.

I know there is a mechanism to add custom types but they have to extend PGobject which is not possible for Java’s InetAddress.

@davecramer

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

Seems reasonable, I'll add it to the list...

@davecramer davecramer added the easy label Mar 6, 2018

@vlsi

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

PGObject -> InetAddress change would break backward compatibility.

I think getObject(..., InetAddress.class) would be sane though.
@maffe , what do you think ?

@maffe

This comment has been minimized.

Copy link
Author

commented Mar 6, 2018

You’re right, that’s not backward compatible. Maybe this behaviour could be configurable, but I’m not sure if that’s worth the effort.

I think it’s not easily possible to use getObject(..., InetAddress.class) in cases where you’re working with an EntityManager and createNativeQuery(…) because the returned Query object basically only offers getSingleResult()/getResultList() with no way to request certain types (like InetAddress).

@vlsi

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

@maffe , would you have some luck with createNativeQuery(String sqlString, Class resultClass)?

PS. Have you tried https://github.com/The-Alchemist/hibernate-postgresql/blob/master/src/main/java/com/github/thealchemist/pg_hibernate/InetAddressType.java ?

@maffe

This comment has been minimized.

Copy link
Author

commented Mar 7, 2018

@vlsi Thank you for your suggestions. I have tried

em.createNativeQuery("SELECT '127.0.0.1'::inet", InetAddress.class)
    .getSingleResult();

but that results in an exception:

org.eclipse.persistence.exceptions.QueryException:
Exception Description: Missing descriptor for [class java.net.InetAddress].
Query: ReadAllQuery(referenceClass=InetAddress sql="SELECT '127.0.0.1'::inet")
at org.eclipse.persistence.exceptions.QueryException.descriptorIsMissing(QueryException.java:478)
at org.eclipse.persistence.queries.ObjectLevelReadQuery.checkDescriptor(ObjectLevelReadQuery.java:831)
at org.eclipse.persistence.queries.DatabaseQuery.execute(DatabaseQuery.java:824)
at org.eclipse.persistence.queries.ObjectLevelReadQuery.execute(ObjectLevelReadQuery.java:1134)
at org.eclipse.persistence.queries.ReadAllQuery.execute(ReadAllQuery.java:460)
at org.eclipse.persistence.queries.ObjectLevelReadQuery.executeInUnitOfWork(ObjectLevelReadQuery.java:1222)
at org.eclipse.persistence.internal.sessions.UnitOfWorkImpl.internalExecuteQuery(UnitOfWorkImpl.java:2896)
at org.eclipse.persistence.internal.sessions.AbstractSession.executeQuery(AbstractSession.java:1857)
at org.eclipse.persistence.internal.sessions.AbstractSession.executeQuery(AbstractSession.java:1839)
at org.eclipse.persistence.internal.sessions.AbstractSession.executeQuery(AbstractSession.java:1804)
at org.eclipse.persistence.internal.jpa.QueryImpl.executeReadQuery(QueryImpl.java:258)
at org.eclipse.persistence.internal.jpa.QueryImpl.getSingleResult(QueryImpl.java:521)
…

As for the linked InetAddressType, I use EclipseLink so that is not applicable.

Feel free to close this issue if backward compatibility should be kept. However, as a user I wouldn’t mind if a new major version would include this change.

@feinstein

This comment has been minimized.

Copy link

commented May 21, 2019

Currently we can use UUID with getObject and setObject. It would be very nice to have something like this as well for inet.

@davecramer

This comment has been minimized.

Copy link
Member

commented May 21, 2019

The difference is that there is a built-in UUID java type. That said I see no reason not to return the InetAddress from getObject. PR's are welcome

@feinstein

This comment has been minimized.

Copy link

commented May 21, 2019

There's also a built-in InetAddress Java type.

I would submit a PR if I were more experienced with jdbc inner workings, but I have no idea on how to code this modification.

@myowaithant9

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@davecramer This issue has solved and merged in PR #1527 So, shall we close this issue?

@feinstein

This comment has been minimized.

Copy link

commented Jul 24, 2019

Are there any examples on how to use it?

@myowaithant9

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

For me, I created separate JPA project with an EntityManager and createNativeQuery(…) to test.

factory = Persistence.createEntityManagerFactory(PERSISTENCE_UNIT_NAME);
EntityManager em = factory.createEntityManager();
final Query query = em.createNativeQuery("SELECT '127.0.0.1'::inet");
final List<Object> resultlist = query.getResultList();
@feinstein

This comment has been minimized.

Copy link

commented Jul 24, 2019

I don't use JPA, so I am not familiar with it. I assume this query should return an InetAddress object right? Did you test passing an InetAddress for insertions as well?

@myowaithant9

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

At first, it returns PG Object. Then, I changed code to return InetAddress. While testing, I've just created a database with column type inet, insert data manually and tested select query in JPA to get InetAddress automatically by looking at the type. That's the main point of this issue, isn't it? Or do I still need to test for insertions as well? I thought that select query is enough for testing.

@feinstein

This comment has been minimized.

Copy link

commented Jul 24, 2019

I think this would be very interesting, since we can map objects directly to the database, both ways.

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Seems we were a little hasty with this solution as we now get host not found exception with networks that are stored in INET columns

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Ouch. Yes that's happening as inet isn't just an address. It's an address + net mask with the mask removed in the text representation if it's the entire address: https://www.postgresql.org/docs/current/datatype-net-types.html

I think all you'd need to do is check for a slash ("/") and if it's present, only use the text up to the slash. There should not be any actual host name resolution, the current error is happening because the slash in the text value makes Java not treat it as an IP address and attempt name resolution.

@maffe

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

I think all you'd need to do is check for a slash ("/") and if it's present, only use the text up to the slash.

But then there’s no way to retrieve the netmask (which seems to be important, otherwise one wouldn’t store it in the database and the problem wouldn’t occur).

Maybe mapping to InetAddress is not a good idea after all …

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

ya, I was actually thinking if there was a slash then just return it as is. Do not attempt hostname resolution

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Ya, I'm thinking we want to revert this, at a minimum make hostname resolution optional as I'm sure it causes a performance regression

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I think it's fine to return just the address portion if someone invokes it via ResultSet.getObject("name", InetAddress.class). In that case the user is explicitly asking for just the InetAddress. That code could check for the slash and strip it if necessary. There should not be any reverse host lookup as you're just wrapping an address in an InetAddress right?

For the generic getObject("name") it's likely better to leave it as a string with the full address/mask.

Third option would be adding a PGInet type that has the split out address / mask. Anybody that wants to use that could use that rather than doing the text splitting themselves. Could add this as a follow up as it'd be a separate / new feature.

@maffe

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

Ya, I'm thinking we want to revert this, at a minimum make hostname resolution optional as I'm sure it causes a performance regression

I think InetAddress does not do any hostname resolution if it’s given an IP address string (without a netmask suffix).

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Ya, I'm thinking we want to revert this, at a minimum make hostname resolution optional as I'm sure it causes a performance regression

I think InetAddress does not do any hostname resolution if it’s given an IP address string (without a netmask suffix).

Reverse name resolution means that for any IP address, the host associated with the IP address is returned.

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Reverse name resolution means that for any IP address, the host associated with the IP address is returned.

There's no reverse name resolution if the value is a literal IP address (which it should be): https://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getByName(java.lang.String)

Should work for both v4 and v6.

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Oh so you are saying because there is a slash it decides that it isn't a literal value... how stupid of it.

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Yep. It wouldn't be a valid name but it probably tries anyway it it's not a valid v4 or v6 address (without a mask). That's why you end up with the name resolution error.

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

FFS... so the real solution is to create a PgInet type as you said and return it as either a host or a network.... To fix this I agree with the above solution. Who wants to provide a PR ?

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I can put something together for this. Will do it in two steps.

First:

  • Revert to returning text value for inet when no type is specified (so no breaking change)
  • Have getObject(..., InetAddress.class) handle values with net masks (so split on slash)

Second:

  • Add PGInet class with separate address / mask fields.
  • Add getObject(...) handling for it

Second piece might need some more thought as not sure if we want to have it be a breaking change and have ResultSet.getObject("some_field") return it by default for inet fields when no type is specified. May also be some open questions on what to put for the mask if there's none included (null? implied value of address length?).

@maffe

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

May also be some open questions on what to put for the mask if there's none included (null? implied value of address length?).

I think it’s reasonable to just return the netmask as a primitive type even if it’s 32 (for IPv4) or 128 (for IPv6). There’s nothing wrong with these values. This avoids NullPointerExceptions in the caller code and unnecessary null-checking.

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I can put something together for this. Will do it in two steps.

First:

  • Revert to returning text value for inet when no type is specified (so no breaking change)
  • Have getObject(..., InetAddress.class) handle values with net masks (so split on slash)

Second:

  • Add PGInet class with separate address / mask fields.
  • Add getObject(...) handling for it

Second piece might need some more thought as not sure if we want to have it be a breaking change and have ResultSet.getObject("some_field") return it by default for inet fields when no type is specified. May also be some open questions on what to put for the mask if there's none included (null? implied value of address length?).

Ya, lets talk about the 2nd step, but the 1st step is good

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

See #1568 for the first half of this. Leaving the second piece for another day in case anybody else wants to take a stab at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.