Skip to content

Conversation

rozza
Copy link
Member

@rozza rozza commented Jan 27, 2023

Help users when using a BsonId in a constructor and there is no known Id property in the model.

JAVA-4838

Help users when using a BsonId in a constructor and there is no
known Id property in the model.

JAVA-4838
@rozza rozza requested a review from jyemin January 27, 2023 15:05
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

This doesn't match the description for JAVA-4838. Is what's outlined there not possible or is there another PR coming? If the former, let's change the ticket description to match this more limited scope of work.

@rozza
Copy link
Member Author

rozza commented Jan 27, 2023

Ah I thought I had added a strike through of the original idea.

I have clarified the ticket and the reason not to support it for decoding.

@jyemin jyemin self-requested a review January 27, 2023 16:04
private final long longField;

@BsonCreator
public CreatorConstructorNoKnownIdModel(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the difference between this and the similar

  @BsonCreator
  public CreatorConstructorIdModel(@BsonId final String id, 
                                   @BsonProperty("integersField") final List<Integer> integerField,
                                   @BsonProperty("longField") final long longField)

just that in this existing one the property is named id and the BsonId annotation is ignored? Is that why this one doesn't throw an exception?

I wonder why we allowed ElementType.PARAMETER as a target for BsonId in the first place... This is the only place I see it used as a parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

CreatorConstructorIdModel has a public getId() method. The PojoCodec considers either id or _id properties as the id property. So in that case it can map to id.

I believe parameter support was added via a PR in JAVA-2599.

To be fair this is an edge case scenario and prevents a possible NPE.

@rozza rozza requested a review from jyemin January 27, 2023 16:18
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

ok, i get it now. LGTM

@rozza rozza merged commit b0f05c8 into mongodb:master Jan 27, 2023
@rozza rozza deleted the JAVA-4838 branch January 27, 2023 16:24
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