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

Object Attributes #1198

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

bdgould
Copy link

@bdgould bdgould commented Nov 24, 2024

I keep getting MergeRequestEvent having a null objectAttributes field. It looks like the API actually uses the syntax: object_attributes. This seems to fix it. There could be other instances of elsewhere in the API.

I'm trying create a simple webhook trigger using a managed GitLab CE 17.5.1 server, and am getting event json like: sample.json

@jmini
Copy link
Collaborator

jmini commented Nov 28, 2024

How are you using this library?

Because the convention of turning underscore notation to camelCase should work out of the box, without us having to redefine it everywhere using a @JsonProperty property annotation.

This is also covered by Unit Tests here:
https://github.com/gitlab4j/gitlab4j-api/blob/0a1b6e0dcc926bb9fa1fb284d12b89388f446548/src/test/java/org/gitlab4j/api/TestGitLabApiEvents.java

So I guess that you have a different jackson config (maybe a different ObjectMapper instance).

@jmini jmini deleted the branch gitlab4j:main November 28, 2024 19:53
@jmini jmini closed this Nov 28, 2024
@jmini
Copy link
Collaborator

jmini commented Nov 29, 2024

When I have deleted the 6.x branch (see #926 (comment)) this PR was closed, but this is not my intention. It can be changed to target main (which is the branch where the next 6.0.0 version is prepared).

@jmini jmini reopened this Nov 29, 2024
@jmini jmini changed the base branch from 6.x to main November 29, 2024 07:03
@jmini
Copy link
Collaborator

jmini commented Nov 29, 2024

I think the configuration is done here:

objectMapper.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);

To configure the ObjectMapper that does the case transformation inside a Quarkus project you need to do this:

import javax.enterprise.inject.Instance;
import javax.inject.Singleton;

import org.gitlab4j.api.utils.JacksonJson;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;

import io.quarkus.jackson.ObjectMapperCustomizer;

public class ObjectMapperConfiguration {

	@Singleton
	ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {
		return new JacksonJson().getObjectMapper().enable(SerializationFeature.INDENT_OUTPUT);
	}
}

I am almost certain other frameworks have similar approaches.

Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

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

I am not against having more information in Annotations on the class, to make it independent from a potential Jackson config.

To accept this Pull Request, work is needed:

  1. add the annotation everywhere needed (not just on few fields) --> consistency is very important
  2. modify the test so that they run without this specific Jackson config (and they have to stay green).

@bdgould
Copy link
Author

bdgould commented Dec 5, 2024

Hi @jmini , Sorry for the delay! I agree, I think the metadata would be helpful -- I was just tied up during the Thanksgiving holiday here in the U.S. and then some unrelated stuff. I can work through it a bit more here in the coming weeks!

I ran into the issue in a spring-boot application I was working through. I did quickly find the spring jackson configuration, however I ran into a somewhat related issue in that some of the event triggers for the webhook used multiple formats of Date/times... This would also be solved potentially with these style annotations. I'll take a broader swing at this!

@jmini
Copy link
Collaborator

jmini commented Dec 16, 2024

What a work! I will review ASAP and merge.

@bdgould
Copy link
Author

bdgould commented Dec 17, 2024

Ha, thanks @jmini. I may need another weekend or two to test a bit more. I tried a couple different things with our managed gitlab server and found a couple date formats were wrong -- I'd like to try a test a bunch more!

@jmini
Copy link
Collaborator

jmini commented Dec 24, 2024

@bdgould it could even be that some date in the test JSON files in the gitlab4j-models/src/test/resources/org/gitlab4j/models folder not really matches what gitlab is sending...

@bdgould
Copy link
Author

bdgould commented Dec 27, 2024

Yeah, I found a couple of slight issues like that. I'm just trying various things with my managed gitlab instance to verify so far.

@jmini
Copy link
Collaborator

jmini commented Dec 30, 2024

I fixed some more cases where the date is sent as 2018-07-01 by the server and where we were serializing as 2018-07-01T00:00:00Z in PR #1223

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