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

[oauth] Fix InaccessibleObjectException #3449

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Mar 12, 2023

This fixes a regression from #3083 and #2994 since JDK internals are now strongly encapsulated. The first mentioned PR removed the type adapter for LocalDateTime, which ultimately lead to the issue when Java 17 was introduced.

Since type adapters already exist for Instant, the InaccessibleObjectException has been fixed by refactoring remaining LocalDateTimes to Instant.

JAR: org.openhab.core.auth.oauth2client-4.0.0-SNAPSHOT.jar

Fixes #3447

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh4-0-0-snapshot-and-m1-google-tts-exception/145046/17

@wborn wborn added the bug An unexpected problem or unintended behavior of the Core label Mar 12, 2023
@wborn
Copy link
Member

wborn commented Mar 13, 2023

the InaccessibleObjectException has been fixed by refactoring remaining LocalDateTimes to Instant.

Will this change cause everybody having to setup new tokens or can it be made backwards compatible?

@jlaur
Copy link
Contributor Author

jlaur commented Mar 13, 2023

Will this change cause everybody having to setup new tokens or can it be made backwards compatible?

Backwards compatibility should already be ensured by fallback to LocalDateTime:

.registerTypeAdapter(Instant.class, (JsonDeserializer<Instant>) (json, typeOfT, context) -> {
try {
return Instant.parse(json.getAsString());
} catch (DateTimeParseException e) {
return LocalDateTime.parse(json.getAsString()).atZone(ZoneId.systemDefault()).toInstant();
}
})

@jlaur jlaur force-pushed the 3447-oauth-fix-serialization branch from 3f6a1e2 to 510c75c Compare March 13, 2023 22:00
Signed-off-by: Jacob Laursen <[email protected]>
@jlaur jlaur force-pushed the 3447-oauth-fix-serialization branch from 510c75c to 4d65083 Compare March 13, 2023 22:00
@jlaur jlaur marked this pull request as ready for review March 14, 2023 21:38
@jlaur jlaur requested a review from a team as a code owner March 14, 2023 21:38
@jlaur jlaur marked this pull request as draft March 14, 2023 22:13
@jlaur
Copy link
Contributor Author

jlaur commented Mar 14, 2023

I have tested with the Miele Cloud binding.

StorageHandler.For.OAuthClientService.json differences

Before: (4.0 and 3.4.x)

  "[email protected]": {
    "class": "java.lang.String",
    "value": "{\n  \"date\": {\n    \"year\": 2023,\n    \"month\": 3,\n    \"day\": 14\n  },\n  \"time\": {\n    \"hour\": 23,\n    \"minute\": 47,\n    \"second\": 1,\n    \"nano\": 502530800\n  }\n}"
  },

Now: (this PR)

  "[email protected]": {
    "class": "java.lang.String",
    "value": "\"2023-03-14T22:56:01.498029Z\""
  },

AccessTokenResponse was already using a well-formatted Instant string for createdOn.

For backwards compatibility we might need to be able to deserialize something like "{\n "date": {\n "year": 2023,\n "month": 3,\n "day": 14\n },\n "time": {\n "hour": 23,\n "minute": 47,\n "second": 1,\n "nano": 502530800\n }\n}" also.

@jlaur
Copy link
Contributor Author

jlaur commented Mar 15, 2023

For backwards compatibility we might need to be able to deserialize something like "{\n "date": {\n "year": 2023,\n "month": 3,\n "day": 14\n },\n "time": {\n "hour": 23,\n "minute": 47,\n "second": 1,\n "nano": 502530800\n }\n}" also.

It seems to be handled here already (now changed to Instant):

try {
LocalDateTime lastUsedDate = gson.fromJson(value, LocalDateTime.class);
return lastUsedDate;
} catch (Exception e) {
logger.info("Unable to deserialize json, reset LAST_USED to now. json:\n{}", value);
return LocalDateTime.now();
}

@jlaur
Copy link
Contributor Author

jlaur commented Mar 15, 2023

The exceptions reported in the linked issue are fixed, so I'm marking this as ready for review. There are still some issues being sorted out (see details in the issue), but at this point I think it's unrelated and should not block the fixes provided here any longer.

@jlaur jlaur marked this pull request as ready for review March 15, 2023 22:15
@jlaur
Copy link
Contributor Author

jlaur commented Mar 16, 2023

The fix has now been tested and confirmed by the user.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@wborn wborn merged commit 3e1c0b3 into openhab:main Mar 16, 2023
@wborn wborn added this to the 4.0 milestone Mar 16, 2023
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Jacob Laursen <[email protected]>
GitOrigin-RevId: 3e1c0b3
@jlaur jlaur deleted the 3447-oauth-fix-serialization branch July 31, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[oauth] Error fetching access token. Invalid oauth code? Please generate a new one.
3 participants