@Nullable message fields

[ProRoman]

Hi

After moving to the sdk 3.x I noticied that all message fields are marked as @nullable

I believe that it is not valid for some fields for example message.id (sender/from, to also) because at the same time the sdk requires @nonnull id in all requests to the API.

What do you think?
AB#9534

[baywet]

Hi @ProRoman
Thanks for reaching out.

The nullable nonnull annotations are something we've wanted feedback on since the early previews of v3. To set some context: there were no annotations before v3 leading the compiler to "guess" the right information for kotlin developers. And it'd often guess wrong due to the complexity of the SDK, making kotlin developer's lives harder (also applies to java developers with advanced linting tools like checkbugs).

More of the tagging (either in the core SDK or in the generated part of the library) has been made through "precision-guessing" due to the volume of annotations to add, and some annotations might be wrong.
My understanding is that a nonnull annotation (for a return value) makes things easier for consumers as they don't need to check for null, so the more nonnull annotations we can add where it makes sense, the better.

Now, if we specifically focus on the case of the id field. Whenever the API returns an entity, it will always have a id, leading us to think we could tag it as nonnull.
However if I do the following:

final Message message = new Message();
sysout(message.id); // <-- null value here

As the value can be null throughout the lifecycle of the application, we tagged it as nullable. Does that make sense?

As for the methods that accept an id as a parameter, this is to build the URL for the request, and a null value would be invalid, this is why the parameter is tagged as nonnull.

Let me know what you think. I'm happy to review things if one of my assumptions is flawed.

Another think I'd like you to provide feedback on while we're at it, is the nullable tag of the "buildRequest" methods We've got some internal feedback they should be tagged as nonnull. What are your thoughts?
If we change it from nullable to nonnull, would you, as a consumer, consider this a breaking change?

[ProRoman]

@baywet Thanks! I understood your case

about your question

now we use this when use the "buildRequest"
Example:

MessageCollectionRequest messageCollectionRequest = folder().messages().buildRequest();
Assert.notNull(messageCollectionRequest, "MessageCollectionRequest must not be null");

According to code buildRequest com.microsoft.graph.http.BaseCollectionRequestBuilder#buildRequest(java.util.List<? extends com.microsoft.graph.options.Option>) can not return null: object or exception.

[baywet]

thanks for the additional information.
So you'd vote to have the annotation changed on that method?
Would you consider changing the annotation a breaking change?

[ProRoman]

For me it will not breaking change.

[baywet]

wouldn't it force you to remove the assert? i.e. if you upgrade the dependency and the nullable changed to nonnull, will your code still compile if you don't change it?

[baywet]

To recap discussions that happened internally, we'll be changing the annotations to nonnull for the buildRequest methods. Transferring the issue to core where it belongs.

[baywet]

I just authored #214 which fixes this issue and will close it once it gets merged.